Re: [PATCH] trailers: stop recognizing URLs as trailers

[Junio C Hamano <gitster@pobox.com>]

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> An HTTPS URL starts with an alphanumeric scheme followed by a colon.
> That means that they will be recognized as trailers in a trailer block.
> That turns out to be a problem in practice. Let’s stop recognizing these
> as trailers by failing the trailer parsing when we:
>
> 1. find the separator;
> 2. the separator and the next two characters form `://`; and
> 3. we haven’t parsed any whitespace yet.

When I read the problem description, I would have expected you to
say "If we find <token>: at the beginning of the line, check <token>
against known URL schemes like https, ftp, etc. and declare that the
line is not a trailer, if it matches".  Checking against "://" is
much more robust, as it is less likely to happen in random text, and
we avoid maintaining a whitelist of scheme names.  You are certainly
smarter than I am ;-).

Shouldn't we restrict the token preceding "://" more strictly than
simply prohibiting whitespace?

> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---

> diff --git a/trailer.c b/trailer.c
> index 6d8ec7fa8d8..971ae459596 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -635,8 +635,13 @@ static ssize_t find_separator(const char *line, const char *separators)
>  	int whitespace_found = 0;
>  	const char *c;
>  	for (c = line; *c; c++) {
> -		if (strchr(separators, *c))
> +		if (strchr(separators, *c)) {
> +			/* avoid accidental URL matches (://) */
> +			if (*c == ':' && c[1] == '/' && c[2] == '/' &&

How do we know the references to c[1] and c[2] do not access an
unmapped piece of memory?  The answer is that line[] is NUL
terminated, so c[0] == ':' guarantees that c[1] is safe to read and
unless it is NUL (and c[1] =='/' certainly means it is not NUL),
c[2] is safe to read.

OK.  Makes sense to me.

Thanks.

> +			    !whitespace_found)
> +				return -1;
>  			return c - line;
> +		}
>  		if (!whitespace_found && (isalnum(*c) || *c == '-'))
>  			continue;
>  		if (c != line && (*c == ' ' || *c == '\t')) {