Re: [RFC] imap-send: escape backslash in password

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2017, 19:09 UTC
Message-ID: <xmqqbmnvtain.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <58b783d6-c024-4491-2f88-edfb9c43c55c@morey-chaisemartin.com>

Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:

> Password containing backslashes need to have them doubled to have them properly interpreted by the imap server.

Please wrap this into lines with reasonable lengths like 72 cols.

Is the quoting rules documented somewhere? If so, please also give a reference to it here. RFC3501 "6.2.3 LOGIN Command" does not say much (other parts of the RFC may specify the rules that apply to arguments in general, but I didn't look for them). Without such reference, it is hard to judge if this change is sufficient or even correct (in an extreme case, the IMAP server you are talking with that prompted you to make this change might be in violation).

For example, FRC3501 "9. Formal Syntax" says that both "password" and "userid" are "astring"; it looks strange that the code with this patch only touches cred.password while sending cred.username as-is.

> +static char* imap_escape_password(const char *passwd)

In our codebase, asterisk sticks to identifier, not typename.  I.e.

	static char *imap_escape(...)

Show 24 quoted lines

> +{
> +	const unsigned passwd_len = strlen(passwd);
> +	char *escaped = xmalloc(2 * passwd_len + 1);
> +	const char *passwd_cur = passwd;
> +	char *escaped_cur = escaped;
> +
> +	do {
> +		char *next = strchr(passwd_cur, '\\');
> +
> +		if (!next) {
> +			strcpy(escaped_cur, passwd_cur);
> +		} else {
> +			int len = next - passwd_cur + 1;
> +
> +			memcpy(escaped_cur, passwd_cur, len);
> +			escaped_cur += len;
> +			next++;
> +			*(escaped_cur++) = '\\';
> +		}
> +		passwd_cur = next;
> +	} while(passwd_cur);
> +
> +	return escaped;
> +}

I wonder if we should use strbuf here perhaps like so:

	struct strbuf encoded = STRBUF_INIT;
	const char *p;

	for (p = passwd; *p; p++) {
		if (need_bs_quote(*p))
			strbuf_addch(&encoded, '\\');
		strbuf_addch(&encoded, *p);
	}
	return strbuf_detach(&encoded, NULL);

Show 12 quoted lines

>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
>  {
>  	struct credential cred = CREDENTIAL_INIT;
> @@ -1090,7 +1116,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
>  			if (!srvc->user)
>  				srvc->user = xstrdup(cred.username);
>  			if (!srvc->pass)
> -				srvc->pass = xstrdup(cred.password);
> +				srvc->pass = imap_escape_password(cred.password);
>  		}
>  
>  		if (srvc->auth_method) {

Thanks.