On Tue, Jan 09, 2024 at 05:53:34PM +0000, Victoria Dye via GitGitGadget wrote: > While testing 'git fsck' checks on .gitmodules URLs, I noticed that some > invalid URLs were passing the checks. Digging into it a bit more, the issue > turned out to be that 'credential_from_url_gently()' parses certain URLs > (like "http://example.com:something/deeper/path";) incorrectly, in a way that > appeared to return a valid result. I don't think that checks was ever intended to be an overall URL-quality check. The reason we used the credential code in the fsck check is that we were checking for URLs which triggered a specific credential-related vulnerability. I don't mind tightening things further as long as: 1. We are not allowing any cases that the credential code would have forbidden (i.e., something that might let the vulnerability slip through, since ultimately it is the credential code which will need to be protected). You ported over the newline check, which is the main thing. It's possible that there is some difference between the two parsers that may allow an invalid input to create a newline for one but not the other, but having now looked over the code, I don't think so. And I think one could argue that the security-importance of the fsck check has mostly run its course. The real fix was in the credential code itself, and the matching fsck change was mostly about protecting downstream clients until they were upgraded. Now that it's been several years, there's not as much value there. 2. It is not making it harder for users to work with repositories that may contain malformed URLs that _aren't_ vulnerabilities. It sounds like the specific cases you found already don't work at all with Git, so presumably nobody is using them. By making it an fsck check, though, any mistakes that are embedded in history (even if they are now corrected) will make it a pain to use the repository with sites that enable transfer.fsckObjects. My gut feeling is that this is probably OK in practice. If it does cause pain, we might consider loosening the fsck.gitmodulesUrl severity (under the notion from above that it is no longer a critical security check). But if it doesn't cause real-world pain, being pickier is probably better (it may save us from a vulnerability down the road). -Peff
