Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 27, 2020, 03:35 UTC
Message-ID: <20201027033518.GH2645313@google.com>
In-Reply-To: <20201005121645.GG2907394@coredump.intra.peff.net>

Hi,

Jeff King wrote:

Show 18 quoted lines

> However, it's still a reasonable idea to forbid symlinks for these
> files:
>
>   - As noted, they can still be used to read out-of-repo files (which is
>     fairly restricted, but in some circumstances you can probe file
>     content by speculatively creating files and seeing if they get
>     ignored)
>
>   - They don't currently behave well in all cases. We sometimes read
>     these files from the index, where we _don't_ follow symlinks (we'd
>     just treat the symlink target as the .gitignore or .gitattributes
>     content, which is actively wrong).
>
> This patch forbids symlinked versions of these files from entering the
> index. We already have helpers for obscured forms of the names from
> e7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and
> 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which
> were done as part of the series touching .gitmodules.

Thanks again for this. Since this patch has been in "next", we've gotten a little experience of it at Google.

We've been running with the fsck check for a while (more than a year), but not the verify_dotfile check. The verify_dotfile check didn't trigger for anyone with .gitattributes, but it hit several people for .gitignore. Some examples that users have mentioned:

Before https://android-review.googlesource.com/c/platform/tools/test/connectivity/+/1462771/ Android used a .gitignore symlink for two directories that had similar gitignore requirements. Diagnosing the error was confusing for them, especially because the "repo" wrapper tool produced messages like

	error.GitError: Cannot initialize work tree for platform/tools/test/connectivity

Eventually someone manually ran

	$ git add --a
	error: invalid path 'acts_tests/.gitignore'
	error: unable to add 'acts_tests/.gitignore' to index
	fatal: adding files failed

which helped them realize it was a git issue and helped me point them to the need to replace the symlink with a plain file.

As another example, a user working with the https://github.com/bakerstu/openmrn.git repository noticed "git checkout" commands failing. In this user's case, the checkout failed part-way through, producing confusing behavior ("git status" showing entries missing from the index). When I tried to reproduce this, I wasn't able to clone the repository at all because it failed fsck; after disabling transfer.fsckObjects, I still wasn't able to check out HEAD.

Observations:

- since some widely used repositories have .gitignore symlinks, I
  think we can't forbid it in fsck, alas

- it would be useful to be able to check whether these symlinks would
  not escape the worktree, for a more targeted check.  It might be
  nice to even respect these settings when they would not escape the
  worktree, but not necessarily

- we could use a clearer error message than "invalid path".  There's
  some room for improvement in "git checkout"'s error handling, too
  --- I think my ideal would be if the operation would fail entirely,
  with an advice message describing a checkout command that would
  succeed (But how do I checkout another commit while excluding some
  files? Should it suggest a sparse checkout?).

That's all I have time for today --- will revisit again tomorrow.

Thoughts?

Thanks, Jonathan