Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2024, 17:28 UTC
Message-ID: <xmqqy16r1ulu.fsf@gitster.g>
In-Reply-To: <20240626132440.3762363-2-csokas.bence@prolan.hu>

"Csókás, Bence" <csokas.bence@prolan.hu> writes:

Show 5 quoted lines

> Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '
> etc. lines mess with git-send-email. In parsing the mbox headers,
> this is handled by calling `sanitize_address()`. This function
> is called when parsing the message body as well, but was only
> used for comparing it to $author. Now we add it to @cc too.

The above is misleading, though. We do use sanitize_address on addresses we find on e-mail headers, but the result is not used in @to or @cc, the addresses we use to actually send the message out.

Perhaps phrase it more like ...

    When we check addresses found on the mbox headers to see if we
    want to add them to Cc:, we use sanitize_address() function to
    normalize the addresses before passing them to the suppression
    mechanism, but we use the original addresses for the purpose of
    sending the message out.

    We use the same logic on the address-looking strings found on
    trailer lines that appear in the message body.  Sanitized
    addresses are used for Cc-suppression purposes, but the original
    addresses as written by the end-user are used as the mail
    destination.

    There are certain quoting rules for e-mail addresses, and unlike
    addresses on e-mail headers that are generated by format-patch,
    hand-written addresses on the trailer lines are more likely to
    violate them by mistake.

    When adding the address found on a trailer line in the message
    body, use the sanitized address for both sending the message
    out, as well as checking with Cc-suppression mechanism, to
    reduce the risk of malformed hand-written addresses to get the
    message rejected (but keep using the original addresses found on
    the e-mail headers in the input message for e-mail destination).

... or something like that?

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 58699f8e4e..7e0b8ae57c 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh

This test uses mixture of test_grep and grep, so use of "grep" below is fine (but in the longer term we should make sure the tests use the former for better debuggability).

As I always say, we should resist the temptation to write our tests just to demonstrate our shiny new toy. In this case, the test is too focused to show that the system will give a best-effort output when fed invalid and/or malformed addresses, but it does not see what happens to a well formed addresses (ideally they are passed intact, but is that what happens with the new code?). Perhaps add one or two trailer lines with valid addresses (and non-address, like "BugId: 143421", that should not appear at all in the output) on them?

Thanks.

Show 19 quoted lines

> +test_expect_success $PREREQ 'cc list is sanitized' '
> +	clean_fake_sendmail &&
> +	test_commit weird_cc_body &&
> +	test_when_finished "git reset --hard HEAD^" &&
> +	git commit --amend -F - <<-EOF &&
> +	Test Cc: sanitization.
> +
> +	Cc: Person, One <one@example.com>
> +	Reviewed-by: Füñný Nâmé <odd_?=mail@example.com>
> +	Signed-off-by: A. U. Thor <thor.au@example.com>
> +	EOF
> +	git send-email -1 --to=recipient@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" >actual-show-all-headers &&
> +	test_cmp expected-cc commandline1 &&
> +	grep "^(body) Adding cc: \"Person, One\" <one@example.com>" actual-show-all-headers &&
> +	grep "^(body) Adding cc: =?UTF-8?q?F=C3=BC=C3=B1n=C3=BD=20N=C3=A2m=C3=A9?="\
> +" <odd_?=mail@example.com>" actual-show-all-headers &&
> +	grep "^(body) Adding cc: \"A. U. Thor\" <thor.au@example.com>" actual-show-all-headers
> +'

>  test_expect_success $PREREQ 'sendemail.composeencoding works' '
>  	clean_fake_sendmail &&
>  	git config sendemail.composeencoding iso-8859-1 &&