git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore
Date: Tue, 27 Oct 2020 16:43:09 -0700	[thread overview]
Message-ID: <20201027234309.GA1298045@google.com> (raw)
In-Reply-To: <20201027075853.GH3005508@coredump.intra.peff.net>

Jeff King wrote:
> On Mon, Oct 26, 2020 at 08:35:18PM -0700, Jonathan Nieder wrote:

>> Observations:
>>
>> - since some widely used repositories have .gitignore symlinks, I
>>   think we can't forbid it in fsck, alas
>
> I am a little puzzled here. You said you had the fsck checks for the
> last year, so why did they just come up now? I guess nobody sets
> transfer.fsckObjects, and because you were testing only with clients,
> your server implementation didn't reject pushes?
>
> I agree it's annoying for them if they fail fsck, but it's not entirely
> a show-stopper. There are config options for fine-tuning what you're
> willing to enforce or ignore. But they of course are also annoying to
> use, because every receiver of a transfer needs to set them (on GitHub,
> for example, you have to email Support).

Yes.  And to reiterate the point a little: the reason nobody sets
transfer.fsckObjects is that we haven't made it easy to distinguish
between "hard error, should never be overridden" checks (like
BAD_PARENT_SHA1), "new tools shouldn't write these but they exist in
important repos like perl.git and anything consuming Git repositories
needs to cope with them" (like MISSING_SPACE_BEFORE_DATE from some
commits' concatenated authors), and so on.

> So I won't be too devastated to remove the symlink checks, or possibly
> downgrade them to purely warnings (or "info"; the naming in fsck.c is
> confusing, because the transfer operations take even warnings as fatal.
> I suspect we could do with some cleanup there).

Downgrading the .gitignore check to warning sounds okay.  .gitmodules
would still want to be an error, of course.

>> - 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
>
> I actually wrote a patch several years ago for checking symlinks (not
> just these ones, but _any_ symlinks in the repo, but of course it would
> be easy to limit it more). It's included at the end of this mail. It's
> been part of my daily build for many years, so I'm confident it doesn't
> crash or have other bad behavior. But it's possible the logic for what
> it catches is faulty.
[...]
> Here's the patch.

Nice!  I'll try to find time to experiment with this.

[...]
>> - we could use a clearer error message than "invalid path".
>
> That part is tricky. The "invalid path" error comes from the caller of
> verify_path(), and we have no way to pass back an intelligent error
> there. We can call error() ourselves, of course. That adds an extra line
> of output, but it's rare enough for verify_path() to fail that it's
> likely OK. However, I would worry that some callers might be surprised
> by it producing output at all.
>
> An alternative is letting the caller pass in a strbuf that we fill out
> with an extra error string.

I think passing in a struct to govern error handling sounds nice.
Alternatively, this might suggest that verify_path is not the right
place for the user-facing check: it's useful because it's exhaustive,
but it may be that we can also catch the same issue earlier and
produce a nicer diagnostic.

>> 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?).
>
> I suspect it's too late for "fail entirely". We may have already written
> to the filesystem, and rolling back is difficult and error-prone. In
> general I'd expect to checkout what we can, produce errors for the rest,
> and let the user work from there with "git status".
>
> But I may be wrong. The problem is loading the value into the index, not
> writing it to the filesystem. So perhaps the relevant code paths load
> the index fully before writing out anything to the filesystem, and it's
> easy to rollback. But I imagine they are using unpack-trees' flag to
> update the filesystem, and I assume that checks out as it loads entries
> (but I didn't confirm).

Right, this would require taking two passes.  I think we do already
perform two passes (first deletions, then additions); I'll have to
look a little more closely to see whether this is straightforward to
do.

I do still like the idea of not respecting .gitignore and
.gitattributes symlinks, by the way.  What the experiences upthread
tell me is just that users need better facilities for repairing
existing repositories.

Thanks,
Jonathan

  parent reply	other threads:[~2020-10-28  1:47 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05  7:17 [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore Jeff King
2020-10-05  7:19 ` [PATCH 1/7] fsck_tree(): fix shadowed variable Jeff King
2020-10-05  7:44   ` Jonathan Nieder
2020-10-05  8:20     ` Jeff King
2020-10-05  8:29       ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 2/7] fsck_tree(): wrap some long lines Jeff King
2020-10-05  7:46   ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 3/7] t7415: rename to expand scope Jeff King
2020-10-05  7:50   ` Jonathan Nieder
2020-10-05  8:24     ` Jeff King
2020-10-05  8:34       ` Jonathan Nieder
2020-10-05  8:49         ` Jeff King
2020-10-05  7:20 ` [PATCH 4/7] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05  7:53   ` Jonathan Nieder
2020-10-05  8:30     ` Jeff King
2020-10-05  8:38       ` Jonathan Nieder
2020-10-05  7:21 ` [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05  8:03   ` Jonathan Nieder
2020-10-05  8:40     ` Jeff King
2020-10-05 21:20       ` Johannes Schindelin
2020-10-06 14:01         ` Jeff King
2020-10-05  7:24 ` [PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-05  8:09   ` Jonathan Nieder
2020-10-05 12:07     ` Jeff King
2020-10-05  7:25 ` [PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-05  8:12   ` Jonathan Nieder
2020-10-05  8:53     ` Jeff King
2020-10-05  7:32 ` [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore Jonathan Nieder
2020-10-05  8:58   ` Jeff King
2020-10-05 12:16 ` [PATCH v2 0/8] " Jeff King
2020-10-05 12:16   ` [PATCH v2 1/8] fsck_tree(): fix shadowed variable Jeff King
2020-10-05 12:16   ` [PATCH v2 2/8] fsck_tree(): wrap some long lines Jeff King
2020-10-05 12:16   ` [PATCH v2 3/8] t7415: rename to expand scope Jeff King
2020-10-05 12:16   ` [PATCH v2 4/8] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05 12:16   ` [PATCH v2 5/8] t7450: test .gitmodules symlink matching against obscured names Jeff King
2020-10-05 12:16   ` [PATCH v2 6/8] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05 12:16   ` [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-27  3:35     ` Jonathan Nieder
2020-10-27  7:58       ` Jeff King
2020-10-27 22:00         ` Junio C Hamano
2020-10-28  9:41           ` Jeff King
2020-10-27 23:43         ` Jonathan Nieder [this message]
2020-10-28 19:18           ` Junio C Hamano
2020-10-05 12:16   ` [PATCH v2 8/8] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-06 20:41   ` [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore Junio C Hamano
2020-10-20 23:19   ` Philip Oakley
2020-10-23  8:17     ` [PATCH] documentation symlink restrictions for .git* files Jeff King
2020-10-23  8:27       ` Jeff King
2020-10-26 22:18       ` Philip Oakley
2020-10-26 22:53         ` Jeff King
2020-10-26 23:32           ` Junio C Hamano
2020-10-27  7:26             ` Jeff King
2020-10-27 18:45               ` Junio C Hamano
2020-10-27 21:00                 ` Philip Oakley
2020-10-28 19:14                   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201027234309.GA1298045@google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).