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

["Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com>]

On Mon, Aug 3, 2026, at 00:36, Junio C Hamano wrote:
> 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 ;-).

The credit for being smart goes to Peff.

https://lore.kernel.org/git/20260609004340.GF358144@coredump.intra.peff.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’t even parsed (1), but just found a line that starts with
   `://`.

In both cases we bail out of the parsing with `-1`, i.e. “not a
trailer”.

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

    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 (-).

🔗 1: https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#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 <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.

I’m mostly a Java programmer so I had the same thought (non-didactically
;) ). 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]