Re: [PATCH v4 5/6] promisor-remote: check advertised name or URL

On Thu, Jan 30, 2025 at 11:51 AM Patrick Steinhardt [off-list ref] wrote:

On Mon, Jan 27, 2025 at 03:48:08PM -0800, Junio C Hamano wrote:

quoted

I wonder if the reader needs to be told a bit more about the
security argument here.  I imagine that the attack vector behind the
use of "secure" in the above paragraph is for a malicious server
that guesses a promisor remote name the client already uses, which
has a different URL from what the client expects to be associated
with the name, thereby such an acceptance means that the URL used in
future fetches would be replaced without the user's consent.  Being
able to silently repoint the remote.origin.url at an evil repository
you control is indeed a powerful thing, I would guess.  Of course,
in a corp environment, such a mechanism to drive the clients to a
new repository after upgrading or migrating may be extremely handy.

I'm still very hesitant about letting the server-side control remote
names at all, as I've already mentioned in previous review rounds. I
think that it opens up the client for a whole lot of issues that should
rather be avoided. Most importantly, it takes control away from the
user, as they are not free anymore to name the remotes however they want
to. It also casts into stone current behaviour because it is now part of
the protocol.

The server-side doesn't control remote names at all in this series.
There is just a match or no match, depending on the value of
promisor.acceptFromServer on the client-side, between what the client
already has configured (for example using the clone -c option) and
what the server advertises.

That being said, I get the point that it may make sense to be "agile"
regarding the promisor remotes. But I think we can achieve that without
having to compromise on either usability or security by using something
like a promisor ID instead.

Thanks for the suggestion and the ideas, but I think that what you
suggest could be discussed and implemented as part of a follow up
patch series. This patch series implements basic checks with
information (name and URL) that already exists on the server side and
might also be available on the client side. For a number of use cases
it is likely enough, and it's also not very complex.

I would be fine with resending the series without this patch, if
that's what is prefered though.