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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 30, 2017, 17:53 UTC
Message-ID: <CAPig+cSEYHfupRt+-0sZK6H3_WcT_=wdNB2FfdYuB6geGQYZ3A@mail.gmail.com>
In-Reply-To: <18c9478b-19fc-69f2-229f-67c05a42d4f5@suse.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.

Show 34 quoted lines

> 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