Re: [PATCH] imap-send: URI encode server folder

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Nov 30, 2017 at 5:07 AM, Nicolas Morey-Chaisemartin
<nmoreychaisemartin@suse.com> wrote:
> URI encode the server folder string before passing it to libcurl.
> This fixes the access to the draft folder on Gmail accounts (named [Gmail]/Drafts)

For someone reading this commit message in the future -- someone who
didn't follow the email thread which led to this patch -- "this fixes"
doesn't say much about the actual problem being addressed. Can you
expand the commit message a bit to make it more self-contained? At
minimum, perhaps show the error message you were experiencing, and
cite (as Daniel pointed out) RFC 3986 and the bit about a "legal" URL
not containing brackets.

Also, a natural question which pops into the head of someone reading
this patch is whether other parts of the URL (host, user, etc.) also
need to be handled similarly. It's possible that you audited the code
and determined that they are handled fine already, but the reader of
the commit message is unable to infer that. Consequently, it might be
nice to have a sentence about that, as well ("other parts of the URL
are already encoded, thus are fine" or "other parts of the URL are not
subject to this problem because ...").

The patch itself looks okay (from a cursory read).

Thanks.

> Reported-by: Doron Behar <doron.behar@gmail.com>
> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
> ---
>  imap-send.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 54e6a80fd..36c7c1b4f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
>  {
>         CURL *curl;
>         struct strbuf path = STRBUF_INIT;
> +       char *uri_encoded_folder;
>
>         if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
>                 die("curl_global_init failed");
> @@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
>         strbuf_addstr(&path, server.host);
>         if (!path.len || path.buf[path.len - 1] != '/')
>                 strbuf_addch(&path, '/');
> -       strbuf_addstr(&path, server.folder);
> +
> +       uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
> +       if (!uri_encoded_folder)
> +               die("failed to encode server folder");
> +       strbuf_addstr(&path, uri_encoded_folder);
> +       curl_free(uri_encoded_folder);
>
>         curl_easy_setopt(curl, CURLOPT_URL, path.buf);
>         strbuf_release(&path);
> --
> 2.15.1.272.g8e603414b