git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git@vger.kernel.org, "brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v2 2/5] mailinfo: warn if CR found in base64/quoted-printable email
Date: Wed, 05 May 2021 12:41:51 +0900	[thread overview]
Message-ID: <xmqqr1il4zk0.fsf@gitster.g> (raw)
In-Reply-To: <45d2c4ab58c4b0c6f0c7790890bbf75eb373f999.1620148732.git.congdanhqx@gmail.com> (=?utf-8?B?IsSQb8OgbiBUcuG6p24gQ8O0bmc=?= Danh"'s message of "Wed, 5 May 2021 00:19:59 +0700")

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> When an SMTP server receives an 8-bit email message, possibly with only
> LF as line ending, some of those servers decide to change said LF to
> CRLF.

s/an SMTP server receives/SMTP servers receive/
s/those servers/them/

> Some mailing list softwares, when receives an 8-bit email message,
> decide to encoding such message in base64 or quoted-printable.

s/encoding/encode/

So the issue is not about CRLF terminating the lines of base64 or QP
(we should treat CRLF and LF terminated lines when unwrapping base64
or QP the same way).  It is about seeing CRLF in the payload after
unwrapping base64 or QP.  It was unclear which one was at issue from
the subject alone.

> If an email is transfered through above mail servers, then distributed
> by such mailing list softwares, the recipients will receive an email
> contains a patch mungled with CRLF encoded inside another encoding.
> Thus, such CR couldn't be dropped by mailsplit. Hence, the mailed patch
> couldn't be applied cleanly. Such accidents have been observed in the wild [1].
>
> Let's give our users some warnings if such CR is found.

Hmph.  It is unclear which one of the following we want our endgame
to be:

 (1) strip silently and apply
 (2) warn but strip and apply
 (3) warn but do not strip, letting the application fail

but let's keep reading.  I suspect (1) and (2) might be error prone,
as the mailpath that may have caused this kind of breakage may not
be under end-user's control.

> +static void summarize_quoted_cr(struct mailinfo *mi, int have_quoted_cr)
> +{
> +	if (have_quoted_cr)
> +		warning("quoted CR detected");
> +}

At this step, it is unclear if it is easier to read to make it the
responsibility of the caller to check for have_quoted_cr, but it
will become clear as we add more condition for the warning in later
steps to let callers unconditionally call this helper and decide
when we want to be silent inside this function.

Have you considered adding a new have_quoted_cr member to "struct
mailinfo"?  After all, the mailinfo struct is not only about end
user preference but contains all information we gleaned out of the
incoming message.

>  static void handle_body(struct mailinfo *mi, struct strbuf *line)
>  {
>  	struct strbuf prev = STRBUF_INIT;
> +	int have_quoted_cr = 0;
>  
>  	/* Skip up to the first boundary */
>  	if (*(mi->content_top)) {
> @@ -1051,6 +1063,8 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)
>  				handle_filter(mi, &prev);
>  				strbuf_reset(&prev);
>  			}
> +			summarize_quoted_cr(mi, have_quoted_cr);
> +			have_quoted_cr = 0;
>  			if (!handle_boundary(mi, line))
>  				goto handle_body_out;
>  		}
> @@ -1081,7 +1095,7 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)
>  						strbuf_addbuf(&prev, sb);
>  						break;
>  					}
> -				handle_filter_flowed(mi, sb, &prev);
> +				handle_filter_flowed(mi, sb, &prev, &have_quoted_cr);
>  			}
>  			/*
>  			 * The partial chunk is saved in "prev" and will be
> @@ -1091,7 +1105,7 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)
>  			break;
>  		}
>  		default:
> -			handle_filter_flowed(mi, line, &prev);
> +			handle_filter_flowed(mi, line, &prev, &have_quoted_cr);
>  		}
>  
>  		if (mi->input_error)
> @@ -1100,6 +1114,7 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)
>  
>  	if (prev.len)
>  		handle_filter(mi, &prev);
> +	summarize_quoted_cr(mi, have_quoted_cr);
>  
>  	flush_inbody_header_accum(mi);
>  
> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
> index 147e616533..d8fdda6bea 100755
> --- a/t/t5100-mailinfo.sh
> +++ b/t/t5100-mailinfo.sh
> @@ -228,4 +228,19 @@ test_expect_success 'mailinfo handles unusual header whitespace' '
>  	test_cmp expect actual
>  '
>  
> +check_quoted_cr_mail() {

SP on both sides of (), i.e.

	check_quoted_cr_mail () {

> +	git mailinfo -u "$@" quoted-cr-msg quoted-cr-patch \
> +		<"$DATA/quoted-cr.mbox" >quoted-cr-info 2>quoted-cr-err &&
> +	test_cmp "expect-cr-msg" quoted-cr-msg &&
> +	test_cmp "expect-cr-patch" quoted-cr-patch &&
> +	test_cmp "$DATA/quoted-cr-info" quoted-cr-info
> +}
> +
> +test_expect_success 'mailinfo warn CR in base64 encoded email' '
> +	sed "s/%%/$(printf \\015)/" "$DATA/quoted-cr-msg" >expect-cr-msg &&
> +	sed "s/%%/$(printf \\015)/" "$DATA/quoted-cr-patch" >expect-cr-patch &&
> +	check_quoted_cr_mail &&
> +	grep "quoted CR detected" quoted-cr-err
> +'
> +
>  test_done

  reply	other threads:[~2021-05-05  3:42 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-21  1:34 [PATCH] mailinfo: strip CR from " Đoàn Trần Công Danh
2021-04-21  2:09 ` Junio C Hamano
2021-04-21  3:32 ` brian m. carlson
2021-04-21 12:07   ` Đoàn Trần Công Danh
2021-04-22  1:10     ` brian m. carlson
2021-05-04 17:19 ` [PATCH v2 0/5] Teach am/mailinfo to process quoted CR Đoàn Trần Công Danh
2021-05-04 17:19   ` [PATCH v2 1/5] mailinfo: avoid magic number in option parsing Đoàn Trần Công Danh
2021-05-04 17:19   ` [PATCH v2 2/5] mailinfo: warn if CR found in base64/quoted-printable email Đoàn Trần Công Danh
2021-05-05  3:41     ` Junio C Hamano [this message]
2021-05-04 17:20   ` [PATCH v2 3/5] mailinfo: skip quoted CR on user's wish Đoàn Trần Công Danh
2021-05-05  4:12     ` Junio C Hamano
2021-05-05 15:53       ` Đoàn Trần Công Danh
2021-05-04 17:20   ` [PATCH v2 4/5] mailinfo: strip quoted CR on users' wish Đoàn Trần Công Danh
2021-05-05  4:27     ` Junio C Hamano
2021-05-04 17:20   ` [PATCH v2 5/5] am: learn to process quoted lines that ends with CRLF Đoàn Trần Công Danh
2021-05-05  4:31   ` [PATCH v2 0/5] Teach am/mailinfo to process quoted CR Junio C Hamano
2021-05-06 15:02 ` [PATCH v3 0/6] " Đoàn Trần Công Danh
2021-05-06 15:02   ` [PATCH v3 1/6] mailinfo: load default metainfo_charset lazily Đoàn Trần Công Danh
2021-05-06 15:02   ` [PATCH v3 2/6] mailinfo: stop parsing options manually Đoàn Trần Công Danh
2021-05-08 10:44     ` Junio C Hamano
2021-05-06 15:02   ` [PATCH v3 3/6] mailinfo: warn if CR found in decoded base64/QP email Đoàn Trần Công Danh
2021-05-08 10:52     ` Junio C Hamano
2021-05-06 15:02   ` [PATCH v3 4/6] mailinfo: allow squelching quoted CR warning Đoàn Trần Công Danh
2021-05-06 15:02   ` [PATCH v3 5/6] mailinfo: allow stripping quoted CR without warning Đoàn Trần Công Danh
2021-05-06 15:02   ` [PATCH v3 6/6] am: learn to process quoted lines that ends with CRLF Đoàn Trần Công Danh
2021-05-08 10:57   ` [PATCH v3 0/6] Teach am/mailinfo to process quoted CR Junio C Hamano
     [not found] ` <cover.1620309355.git.congdanhqx@gmail.com>
2021-05-06 15:02   ` [PATCH v3 2/6] mailinfo: stop parse options manually Đoàn Trần Công Danh
2021-05-06 15:19     ` Đoàn Trần Công Danh
2021-05-09 17:12 ` [PATCH v4 0/6] Teach am/mailinfo to process quoted CR Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 1/6] mailinfo: load default metainfo_charset lazily Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 2/6] mailinfo: stop parsing options manually Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 3/6] mailinfo: warn if CRLF found in decoded base64/QP email Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 4/6] mailinfo: allow squelching quoted CRLF warning Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 5/6] mailinfo: allow stripping quoted CR without warning Đoàn Trần Công Danh
2021-05-09 17:12   ` [PATCH v4 6/6] am: learn to process quoted lines that ends with CRLF Đoàn Trần Công Danh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr1il4zk0.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH v2 2/5] mailinfo: warn if CR found in base64/quoted-printable email' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git