Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

From: Piotr Szlazak <piotr.szlazak@gmail.com>
Date: Oct 17, 2024, 15:59 UTC
Message-ID: <a328b6d5-e604-4517-b041-59db19e1f66c@gmail.com>
In-Reply-To: <ZxEr4+sb4DfmtrKv@nand.local>

On 17.10.2024 17:23, Taylor Blau wrote:

> On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:
>>> I think it would probably make more sense to write this as:
>>>
>>>      data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));

Much better! :-)

Show 27 quoted lines

>> I think we have to treat them as a complete unit, as we don't know which
>> bits were set by independent config lines and which were OR-ed in by
>> ALLOW_ANY.
>>
>> So this case:
>>
>>> Stepping back a moment, I suppose this is handling the case where a user
>>> writes:
>>>
>>>      [uploadpack]
>>>          allowTipSHA1InWant = true
>>>          allowReachableSHA1InWant = true
>>>          allowAnySHA1InWant = false
>>>
>>> and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets
>>> the previous two options.
> Yeah, I think that you and I are in agreement here.
>
>> is the one that Piotr is thinking about. But what about:
>>
>>    [uploadpack]
>>    allowAnySHA1InWant = true
>>    allowAnySHA1InWant = false
>>
>> Right now that pair is a noop, which is what I'd expect. But after the
>> proposed patch, it quietly enables ALLOW_TIP_SHA1 and
>> ALLOW_REACHABLE_SHA1.

Rather not as config file is parsed only once:

https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368

Show 9 quoted lines

>> So I think the code has to stay the same, but we perhaps should document
>> that "allow any" has the user-visible side effect of enabling/disabling
>> the other two.
> That would be a useful direction, I think. Double checking
> git-config(1), there is in deed no mention of allowAnySHA1InWant
> implying the other two options, which seems like a gap that would be
> good to address.
>
> Piotr: what do you think?

I agree. I completely missed to test it like that (which works OK):

[uploadpack]
         allowTipSHA1InWant = true
         allowReachableSHA1InWant = true

EOF

I was always testing with allowAnySHAInWant either set to 'true' or 'false'. But always in place.

And having it set to 'false' was disabling my previously set other allowXyzInWant options. Which was a surprise, as I was considering allowAnySHAInWant as a final level of openness for client-side requests[1]. In contradiction to what Taylor expressed here:

> I'm not sure that the current behavior is actually wrong. The final line

in the example above seems to indicate "do not allow any SHA-1 in the 'wants'", which would indeed imply that the other two options should be set to false as well.

So as suggested I will prepare a patch for documentation, so it will be also clear for others. Should it be done using same thread or new one should be created?

[1] For example client can request not reachable objects, trees, blobs.

Regards,

Piotr