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

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.