Re: [PATCH 3/4] promisor-remote: allow a server to advertise extra fields

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 29, 2025, 15:12 UTC
Message-ID: <CAP8UFD2WBqEiSB4YxoDCPuC7wyV81X-pPrLMJiEsv_kdktdEBg@mail.gmail.com>
In-Reply-To: <xmqq34eapfhk.fsf@gitster.g>

On Tue, Apr 15, 2025 at 12:04 AM Junio C Hamano <gitster@pobox.com> wrote:

> As a description of overall syntax of the protocol, this and ...

[...]

Show 7 quoted lines

> ... this may work, but as we are defining protocol between two
> parties, in order to ensure interoperable reimplementations, we need
> to also specify semantics, what are the defined "fields", and what
> each of them mean.  Proposing nebulous "with this framework your
> imagination is the limit, you can invent anything" may work for
> other parts of the system, but not for the part that is about
> communication between two repositories.

Yeah, we can specify a set of accepted fields and their semantics, and that's what I have done in the next version where only "partialCloneFilter" and "token" are accepted so far.

One issue with this is that we might not know how user systems will work and what they will need. For example if they use a special remote helper they created to access a special remote, we might not know what kind of credentials the underlying system on that remote will need, so they might be tempted to reuse some existing fields that weren't designed for that purpose if we don't make it easy enough to add what they need.

Anyway maybe we can deal with that problem later if it ever happens. The "token" field might be enough or maybe there aren't so many kinds of credentials so adding a few more will be enough. And yeah, it's easier to be more lenient on this later than to restrict things later.

Show 5 quoted lines

> IOW, we shouldn't be internally calling these "extra".  From the
> point of view of "core" they may be "extra", but to the developers
> and certainly to the end-users, they shouldn't be "extra" at all.
> They are all supported parts of the system with defined semantics,
> right?

I used "extra" to mean "additional" and "optional", compared to the "name" and "url" fields which are mandatory.

But yeah I have removed "extra" from all the names in the next version to avoid possible confusion.

Show 9 quoted lines

> Another reason why I hate seeing this nebulous "with this, we can
> send anything extra" is because such a thing will have a wrong
> security posture.  If we truly *need* to be able to carry
> *anything*, we need to make sure how values are quoted/escaped, and
> the code for dequoting/unescaping are robustly written to avoid
> passing malformed input and misinterpreting it as something else,
> which would give a new attack vector.  If we can enumerate supported
> fields, their syntax and their possible values, we can make the
> attack surface a lot smaller.

Yeah, maybe it will help a bit security wise.