Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs

[Neil Mayhew <neil@mayhew.name>]

On 13 Nov 24 17:10, Jeff King wrote:

My previous message crossed with Jeff's, and he already addressed most 
of what I was saying.

 >  2. All of the people who are going to clone your repo, who might need
 >     to follow special instructions.
 >
 >     The only reason this hasn't been a huge pain in practice is that
 >     almost nobody turns on transfer.fsckObjects in the first place. In
 >     theory the people who do turn it on know enough to examine the
 >     objects themselves and decide if it's OK. I don't know how true that
 >     is in practice, though (and certainly it would be nice to turn this
 >     feature on by default, but I do worry about people getting caught up
 >     in exactly these kind of historical messes).

This is what happened in our situation. The person had 
transfer.fsckObjects enabled but didn't realize that this was the cause 
of the error. They assumed that the repo's *current* submodule 
configuration was corrupt and was somehow causing the clone to fail even 
though they tried explicitly turning off submodule recursion.

 > We did add the gitmoduleUrl check to help with malicious URLs. But it
 > was always an extra layer of defense over the real fix, which was in the
 > credential code. It's _possible_ that a newly discovered vulnerability
 > will be protected by the existing fsck check, but I'm a little skeptical
 > about its security value at this point (especially because hardly
 > anybody runs it locally, and protection on the hosting sites isn't that
 > hard to work around).

I also think it's surprising to have fsck check the *content* of blobs 
rather than just the relationships between them, and to give a blob 
named .gitmodules special treatment. It goes against the philosophy of 
"do one thing well". I feel that there should be a separate tool for 
checking repos for security vulnerabilities, and it could be given 
additional capabilities (such as checking the configuration as well as 
the objects).

 > So if it's causing people real pain in practice, I think there could be
 > an argument for downgrading the check to a warning. I don't have a
 > strong feeling that we _should_ do that, only that I don't personally
 > reject it immediately as an option.

Perhaps there could be some additional warnings in the documentation for 
transfer.fsckObjects to make people aware of the potential costs of 
using it, particularly the existence of legacy issues in established 
repos that would prevent cloning unless some of the fsck.<msg-id> values 
are set to warn. The documentation currently just says "see 
fsck.<msg-id>" and in my case, despite being fairly familiar with git, 
that didn't give me enough to go on while investigating this.

It might also help to give some guidance on how to track down the object 
name(s) that fsck lists, for example by using git log --raw --all 
--find-object=<NAME>. This would help a user to make a more informed 
decision on how to handle such a situation if it arises. In our case, 
once we did this it was quickly obvious that the problem was a 
historical error that had since been fixed rather than a current problem.