caldav branch review

[Greg Banks]

G'day,

I read with interest the code which recently appeared at

http://git.cyrusimap.org/cyrus-imapd/log/?h=caldav

and here are my comments.  Disclaimer: I know next to nothing about 
CalDAV but I can read Cyrus code.

commit "Initial CalDAV code, after months of working offline."

I really like method_t compared to the typical imapd way of matching
a command string, but the global array methods[] should probably
be const.

In service_init(), the code which builds serverinfo[] could be better
implemented using struct buf to manage the string.  Ditto the building
of httpd_clienthost[].

In service_main(), the calls to iptostring() use a magic number of 60
where they should probably use sizeof(localip) and sizeof(remoteip).
Also, there should be a space after the "if".

The code in cmdloop() which splits the incoming request line
could be better implemented using the new tok_t utility.

The strchr() call after the comment "Quick filter based on first letter"
seems completely pointless, all it's going to do is slow down the
common case of correct commands by a trivial amount.

If the HTTP method is not found, the error is HTTP_NOT_ALLOWED, but
RFC2616 says it should be HTTP_NOT_IMPLEMENTED.

In parse_target(), that's a remarkably convoluted way to check for
either "http" or "https".  Also, the length check for p_uri->path
is against MAX_MAILBOX_PATH, meaning MAX_MAILBOX_PATH+1 including
the NUL terminator, which is the size of request_target_t.path,
but there are several functions which append a '/' to the path
which checking the length.  This smells like an opportunity for
a buffer overflow.

Also, the use of non-NUL-terminated strings in request_target_t
gives me the willies.

parse_xml_body() checks the Content-Type, but not quite correctly.
It checks for a prefix of "text/xml" or "application/xml", but doesn't
check whether it's full or not.  So it handles

Content-Type: text/xml

and

Content-Type: text/xml; charset="utf-8"

but not

Content-Type: text/xmlfoo

Also, parse_xml_body() checks these in a case-sensitive way, whereas
section 3.7 of RFC2616 says

"The type, subtype, and parameter attribute names are case-insensitive."

This same bug is repeated in several other locations too.

I don't see why parse_xml_body() returns both the xmlDoc and
the root element, when each is trivially obtainable from the other.

http_statusline() builds a static buffer using strcpy(,error_message(code))
whose size is not predictable.  Also it should return const char *.

In xml_response(), xmlDocDumpFormatMemoryEnc() is called with the
parameter format=1, perhaps format=0 might be better to avoid generating
unnecessary spaces on the wire.

Also, looking at the libxml2 source, xmlDocDumpFormatMemoryEnc() handles
errors by returning with buf==NULL, which isn't detected in xml_response().
In particular, it's just barely possible to return with bufsiz!=0 and 
buf==NULL.

target_to_mboxname() does a lot of fiddly string futzing - it might be
easier and safer to implement using struct buf.

In meth_report(), in the REPORT_CAL_MULTIGET case, interesting things will
happen if the "href" property does not contain a '/'.

In caldav_db.c, parse_data() uses strtol() when it should probably use 
strtoul().

In dav_prop.c, add_prop_response() checks the return value of xmlNewChild(),
but if the result is NULL it just syslogs() and then keeps using the NULL
pointer.  Returning might be a better idea, or even assert(resp).

proppatch_todb() does a sprintf() to a stack buffer without any length
checking, using "%s" and a string which appears to come from the wire.
Ditto propfind_fromdb().


commit "Initial CalDAV code, after months of working offline." (number 2)

In configure.in, you want to use the macro PKG_CHECK_MODULES() which runs
pkg-config to get the strings needed for CFLAGS and LIBS for both
libxml2 and libical.  Something like this

ENABLE_CALDAV=no
PKG_CHECK_MODULES(CALDAV,[libical>= 0.44 libxml-2.0>= 2.7.7],[
     ENABLE_CALDAV=yes
])
AC_SUBST(CALDAV_CFLAGS)
AC_SUBST(CALDAV_LIBS)



Commit "Added some minimal CalDAV documentation"

This manually created HTML file needs running through the W3C validator,
as it has several flavours of HTML errors.  For example, there's an <h1>
element between the <head> and <body> elements.  Also, closing tags for
the <body> and <html> elements are missing, as are a lot of what I presume
would be <p> tags.

Also, re the issue described:

+Currently calendar collections must reside under a user's Inbox as
+children of the <tt>#calendars</tt> mailbox. This means that calendar
+data can be viewed with an IMAP client, but doing this
+is <b>strongly</b> discouraged as changes made via IMAP will leave the
+mailbox in a state unsuitable for CalDAV. Future
+development will most likely change this so that calendars reside in
+their own partition and/or hierarchy.

we could just hide #calendars from non-admin IMAP clients in imapd.

commit "Added HTTP compiled code"

Looks good.

commit "Added HTTP/WebDAV/CalDAV/iCalendar specs"

Looks good.

commit "include errstr in transaction context rather than passing as a 
separate"

Using struct transaction_t for this is a great idea, but why make the new
field transaction_t.errstr a const char **?  Wouldn't it be simpler to make
it a const char*, and eliminate the extra variable 'errstr' on the stack in
cmdloop()?  Error handling would be 1 character simpler too, like

if ((r = caldav_open(mailbox, CALDAV_CREATE,&caldavdb))) {
     txn->errstr = error_message(r);
     ret = HTTP_SERVER_ERROR;
     goto done;
}


commit "Remove redundant errstr parameter from do_proppatch()."

Looks good.

commit "Simplify property namespace handling for responses."

Looks good.

commit "Fix DAV:resourcetype responses"

Looks good.

commit "- Fix parsing of "*" and "/" URIs (for which methods are they 
legal, and"

Looks good.  But avoiding a strcmp() for "OPTIONS" by using a single-char
check seems like overoptimisation.

commit "httpd: Added COPY/MOVE methods"

See, this is why that strchr() in cmdloop() is just silly.
Otherwise looks good.

commit "httpd: Remove a TODO (we are already using UNIX namespace)"

Looks good.

commit "httpd: Fix logging level for errors"

Looks good.

commit "httpd: Updated TODO list (with some random thoughts)caldav"

Looks good.


-- 
Greg.