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.
