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

On Mon, Aug 3, 2026, at 00:36, Junio C Hamano wrote:

kristofferhaugsbakk@fastmail.com writes:

quoted

From: Kristoffer Haugsbakk <redacted>

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

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.

quoted

Helped-by: Jeff King [off-list ref]
Signed-off-by: Kristoffer Haugsbakk <redacted>
---

quoted

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]