We now use "*string" for such header fields, for Content-* fields, as used in
the imapserver when responding to FETCH commands. We'll now return NIL for an
absent header, and "" (empty string) if the header value is empty.
The acc.Close() at the end of the fuzzing would find inconsistencies. For
example, message files on disk that aren't in the database file. I don't
understand what is happening there, the database file on disk does have those
messages, and it seems the database file is getting replaced. When running the
same code not as a fuzzing test but as a regular Go test doesn't show the
problem. So it seems to be some interaction with fuzzing. The problem is
"solved" (feels more like side-stepped), by starting each fuzz test with a
clean database. We still open & close the account in each fuzz test, and it
doesn't find consistency problems.
The imapclient needs more changes, like more strict parsing, before it can be a
generally usable IMAP client, these are a few steps towards that.
- Fix a bug in the imapserver METADATA responses for TOOMANY and MAXSIZE.
- Split low-level IMAP protocol handling (new Proto type) from the higher-level
client command handling (existing Conn type). The idea is that some simple
uses of IMAP can get by with just using these commands, while more intricate
uses of IMAP (like a synchronizing client that needs to talk to all kinds of
servers with different behaviours and implemented extensions) can write custom
commands and read untagged responses or command completion results
explicitly. The lower-level method names have clearer names now, like
ReadResponse instead of Response.
- Merge the untagged responses and (command completion) "Result" into a new
type Response. Makes function signatures simpler. And make Response implement
the error interface, and change command methods to return the Response as error
if the result is NO or BAD. Simplifies error handling, and still provides the
option to continue after a NO or BAD.
- Add UIDSearch/MSNSearch commands, with a custom "search program", so mostly
to indicate these commands exist.
- More complete coverage of types for response codes, for easier handling.
- Automatically handle any ENABLED or CAPABILITY untagged response or response
code for IMAP command methods on type Conn.
- Make difference between MSN vs UID versions of
FETCH/STORE/SEARCH/COPY/MOVE/REPLACE commands more clear. The original MSN
commands now have MSN prefixed to their name, so they are grouped together in
the documentation.
- Document which capabilities are needed for a command.
We were first getting UIDs in a transaction with a lock. Then getting the
changes and processing them in a special way. And then processing for qresync
in a new transaction. The special processing of changes is now gone, it seems
to have skipped adding/removing uids to the session, which can't be correct.
The new approach is just using a lock and transaction and process the whole
opening of the mailbox, and not processing any changes as part of the open, and
getting rid of the special "initial" mode processing a mailbox.
Once clients enable this extension, commands can no longer refer to "message
sequence numbers" (MSNs), but can only refer to messages with UIDs. This means
both sides no longer have to carefully keep their sequence numbers in sync
(error-prone), and don't have to keep track of a mapping of sequence numbers to
UIDs (saves resources).
With UIDONLY enabled, all FETCH responses are replaced with UIDFETCH response.
NOTIFY is like IDLE, but where IDLE watches just the selected mailbox, NOTIFY
can watch all mailboxes. With NOTIFY, a client can also ask a server to
immediately return configurable fetch attributes for new messages, e.g. a
message preview, certain header fields, or simply the entire message.
Mild testing with evolution and fairemail.
MIME returns the part headers. If 1 is a message, i.e. a message/rfc822 or
message/global, for example when top-level is a multipart/mixed, we were
returning the MIME headers from the message, not from the part.
We also shouldn't be returning a MIME-Version header or the separating newline
for MIME. Those are for MIME headers of a message, but the "MIME" fetch body
part is always about the part.
Found after looking into FETCH BODY handling for issue #327.
The gmail iOS/Android app were showing mime image parts as (garbled) text
instead of rendering them as image. By returning all the optional fields in the
bodystructure fetch attribute, the gmail app renders the image as expected by
the user. So we now add all fields. We didn't before, because we weren't
keeping track of Content-MD5, Content-Language and Content-Location header
fields, since they aren't that useful.
Messages in mailboxes have to be reparsed:
./mox reparse
Without reparsing, imap responses will claim the extra fields
(content-disposition) are absent for existing messages, instead of not claiming
anything at all, which is what we did before.
Accounts and all/some mailboxes can get their "uid validity" bumped ("./mox
bumpuidvalidity $account [$mailbox]"), which should trigger clients to load all
messages from scratch, but gmail doesn't appear to notice, so it would be
better to remove & add the account in gmail.
For issue #327, also relevant to issue #217.
As required by RFC 2045 (MIME). The 78 byte lines work in practice, except that
SpamAssassin has rules that give messages with 78-byte lines spam points.
Mentioned by kjetilho on irc.
For long searches in big mailboxes, without any matches, we would previously
keep working and not say anything. Clients could interpret this silence as a
broken connection at some point. We now send a "we're still searching" untagged
OK responses with code INPROGRESS every 10 seconds while we're still searching,
to prevent the client from closing the connection. We also send how many
messages we've processed, and usually also how many we need to process in grand
total. Clients can use this to show a progress bar.
Attackers scanning the internet can use it to easily create a database of
hosts, software and versions. Let's not make it too easy to find old versions
that may be vulnerable to potential bugs found in the future. We could try
hiding the name "mox" as well, but the banner will still be identifyable, so
there isn't much point, and the public knowing approximately which software is
running can be useful for debugging.
The ID command in IMAP is used by clients to announce their software and
version. We only respond with our version when the user is authenticated.
There are still ways to discover the version number. But they don't involve
standard banner scanning, so someone would have to specifically target mox. We
could tighten that in the future.
For issue #322, based on email. Thanks everyone for discussing.
We were already generating previews of plain text parts for the webmail
interface, but we didn't store them, so were generating the previews each time
messages were listed.
Now we store previews in the database for faster handling. And we also generate
previews for html parts if needed. We use the first part that has textual
content.
For IMAP, the previews can be requested by an IMAP client. When we get the
"LAZY" variant, which doesn't require us to generate a preview, we generate it
anyway, because it should be fast enough. So don't make clients first ask for
"PREVIEW (LAZY)" and then again a request for "PREVIEW".
We now also generate a preview when a message is added to the account. Except
for imports. It would slow us down, the previews aren't urgent, and they will
be generated on-demand at first-request.
We normally check errors for all operations. But for some cleanup calls, eg
"defer file.Close()", we didn't. Now we also check and log most of those.
Partially because those errors can point to some mishandling or unexpected code
paths (eg file unexpected already closed). And in part to make it easier to use
"errcheck" to find the real missing error checks, there is too much noise now.
The log.Check function can now be used unconditionally for checking and logging
about errors. It adjusts the log level if the error is caused by a network
connection being closed, or a context is canceled or its deadline reached, or a
socket deadline is reached.
Example symptom, when deleting a message in the webmail (which moves to Trash):
l=error m="duplicating message in old mailbox for current sessions" err="link data/accounts/mjl/msg/I/368638 data/accounts/mjl/msg/J/368640: no such file or directory" pkg=webmail
Problem introduced a few weeks ago, where moving messages starting duplicating
the message first, and the copy is erased once all references (in IMAP
sessions) to the old mailbox have been removed.
Initialize store and switchboard first, then open account, and close in reverse
order.
Replace all "CheckClosed" calls with "WaitClosed", future changings will keep
an account reference open for a bit after the last regular close, so we can't
know that an account should be closed during tests.
Remove one parameter from the (still too long) "start test server" function in
imapserver testing code.
And merge the duplicate list of capabilities. We had each on a line for
cross-referencing with the RFC, and all capabilities again but on a single line
to use in the server greeting. Now it's just one list.
Since a recent change (likely since implementing MULTIAPPEND), the temporary
files weren't removed any more. When changing it, I must have had the wrong
mental model about the MessageAdd method, assuming it would remove the temp
file.
Noticed during tests.
It still blocks on reading partial flushes from clients, preventing progress
and eventually timing out. The flate library needs more changes to make this
work.
Connections from iOS mail sometimes timed out, not always.
The extension is simply not announced, code is still present.
Broken since introducing LoginAttempts. The fuzzing functions didn't get the
store.Init() call, and would hang on trying to send to the loginattemptwriter.
Keeping the message files around, and the message details in the database, is
useful for IMAP sessions that haven't seen/processed the removal of a message
yet and try to fetch it. Before, we would return errors. Similarly, a session
that has a mailbox selected that is removed can (at least in theory) still read
messages.
The mechanics to do this need keeping removed mailboxes around too. JMAP needs
that anyway, so we now keep modseq/createseq/expunged history for mailboxes
too. And while we're at it, for annotations as well.
For future JMAP support, we now also keep the mailbox parent id around for a
mailbox, with an upgrade step to set the field for existing mailboxes and
fixing up potential missing parents (which could possibly have happened in an
obscure corner case that I doubt anyone ran into).
We normally recover from those situations, printing stack traces instead of
crashing the program. But during tests, we're not looking at the prometheus
metrics or all the output. Without these checks, such panics could go
unnoticed. Seems like a reasonable thing to add, unhandled panics haven't been
encountered in tests.
DeliverMessage() is now MessageAdd(), and it takes a Mailbox object that it
modifies but doesn't write to the database (the caller must do it, and plenty
of times can do it more efficiently by doing it once for multiple messages).
The new AddOpts let the caller influence how many checks and how much of the
work MessageAdd() does. The zero-value AddOpts enable all checks and all the
work, but callers can take responsibility of some of the checks/work if it can
do it more efficiently itself.
This simplifies the code in most places, and makes it more efficient. The
checks to update per-mailbox keywords is a bit simpler too now.
We are also more careful to close the junk filter without saving it in case of
errors.
Still part of more upcoming changes.
We effectively held the account write-locked by using a writable transaction
while processing the FETCH command. We did this because we may have to update
\Seen flags, for non-PEEK attribute fetches. This meant other FETCHes would
block, and other write access to the account too.
We now read the messages in a read-only transaction. We gather messages that
need marking as \Seen, and make that change in one (much shorter) database
transaction at the end of the FETCH command.
In practice, it doesn't seem too sensible to mark messages as seen
automatically. Most clients probably use the PEEK-variant of attribute fetches.
Related to issue #128.
Writing to a connection goes through the flate library to compress. That writes
the compressed bytes to the underlying connection. But that underlying
connection is wrapped to raise a panic with an i/o error instead of returning a
normal error. Jumping out of flate leaves the internal state of the compressor
in undefined state. So far so good. But as part of cleaning up the connection,
we could try to flush output again. Specifically: If we were writing user data,
we had switched from tracing of protocol data to tracing of user data, and we
registered a defer that restored the tracing kind and flushed (to ensure data
was traced at the right level). That flush would cause a write into the
compressor again, which could panic with an out of bounds slice access due to
its inconsistent internal state.
This fix prevents that compressor panic in two ways:
1. We wrap the flate.Writer with a moxio.FlateWriter that keeps track of
whether a panic came out of an operation on it. If so, any further operation
raises the same panic. This prevents access to the inconsistent internal flate
state entirely.
2. Once we raise an i/o error, we mark the connection as broken and that makes
flushes a no-op.
