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

On Mon, Aug 3, 2026, at 00:36, Junio C Hamano wrote:
> [email protected] writes:
>
>> From: Kristoffer Haugsbakk <[email protected]>
>>
>> An HTTPS URL starts with an alphanumeric scheme followed by a colon.
>> That means that they will be recognized as trailers in a trailer bloc=
k.
>> That turns out to be a problem in practice. Let=E2=80=99s stop recogn=
izing 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=E2=80=99t 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 ;-).

The credit for being smart goes to Peff.

https://lore.kernel.org/git/[email protected].=
net/T/#m03ac1a456648090c04cdf5141b7a3e638f1213d1

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

Right now (with this code) we know that:

1. We have either parsed only alphanumerics and hyphens (whitespace is
   ruled out); or
2. We haven=E2=80=99t even parsed (1), but just found a line that starts=
 with
   `://`.

In both cases we bail out of the parsing with `-1`, i.e. =E2=80=9Cnot a
trailer=E2=80=9D.

Wikipedia[1] tells me that this current check *does* have a false positi=
ve:

    A non-empty scheme component followed by a colon (:), consisting of
    a sequence of characters beginning with a letter and followed by any
    combination of letters, digits, plus (+), period (.), or hyphen (-).

=F0=9F=94=97 1: https://en.wikipedia.org/wiki/Uniform_Resource_Identifie=
r#Syntax

A URL *must* begin with a letter, but a trailer can just be a
digit. Which means that this is not the start of a URL:

    1://

But the current code will reject it as a URL.

There are also other false positives like the strange but legal trailer
key `-`.

Other than that, the character set of trailers (alphanums and hyphens)
is a strict subset of URL <scheme>.

I also see that the git-interpret-trailers(1) doc update should say
alphanumerics and/or hyphens instead of just alphanums.

>
>> Helped-by: Jeff King <[email protected]>
>> Signed-off-by: Kristoffer Haugsbakk <[email protected]>
>> ---
>
>> 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 =3D 0;
>>  	const char *c;
>>  	for (c =3D line; *c; c++) {
>> -		if (strchr(separators, *c))
>> +		if (strchr(separators, *c)) {
>> +			/* avoid accidental URL matches (://) */
>> +			if (*c =3D=3D ':' && c[1] =3D=3D '/' && c[2] =3D=3D '/' &&
>
> 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] =3D=3D ':' guarantees that c[1] is safe to read and
> unless it is NUL (and c[1] =3D=3D'/' certainly means it is not NUL),
> c[2] is safe to read.
>
> OK.  Makes sense to me.

I=E2=80=99m mostly a Java programmer so I had the same thought (non-dida=
ctically
;) ). Yes, because of sentinel `NUL` and boolean short-circuiting we can
incrementally peak one character ahead. This would be wrong in any
language without `NUL` terminating strings, but here it is
correct. Indeed, checking the length first (which you would need to do
in Java) would incur a linear cost since you need to scan the string
until you hit the `NUL` terminator.

>
> Thanks.
>[snip]