Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2015, 22:11 UTC
Message-ID: <xmqqh9v2prvm.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150203215043.GA21357@peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > But wouldn't we still fail writing "foo/bar" at that point if "foo" is a
>> > regular file (again, we should never do this, but that is the point of
>> > the test).
>> 
>> The point of the test is not to create foo, whether it is a symlink
>> or an emulating regular file, in the first place.
>
> I thought the point was not to create "../bar", when "foo" points to
> "..".

That is not what the new test you added was doing, though. You are calling "tmp" in that test "foo" and "../foo" in the test is called "../bar" in the message I am responding to, so that is confusing, but in these tests you added, I do not see anything that prepares a symbolic link ON the filesystem and wait for "git apply" to get fooled.

> I agree that the way you have implemented it is that we would
> never even write "foo", and the test checks for that, but to me that is
> the least interesting bit of what is being tested.

They are both interesting. When rejecting an input, "git apply" must be atomic. When checking an input, "git apply" should notice and reject a patch that tries to follow a symbolic link.

Taken together:

 (1) If a patchset tries to create a symbolic link and then tries to
     follow it, the latter principle should make "git apply" to
     reject the patchset, and the former should make sure there is
     no external effect.

 (2) If a patchset tries to affect a path, and the target of the
     patch application has a symlink that may divert the operation
     to the original path the patchset wants to affect, the latter
     principle should make "git apply" to reject the patchset, too.

And my observation is that the new tests that are not protected by SYMLINKS prerequisite (i.e. all of them) in your new test 4139, are all the former kind. As "git aplly" must be atomic, rejection must be decided without touching the filesystem at all. Hence there is no need for the filesystem to even support symbolic links.

But some bozo may try to break "git apply" in the future and try to incrementally apply the patch in a patchset, and at that point, the existing "test_must_fail git apply" may pass not because we correctly decide not to follow symbolic links but because his broken version of "git apply" would try to create a symbolic link (which we would turn into a regular file) and then the filesystem would fail to follow that symbolic link mimic, and as the result the test may still pass.

In order to prevent that from happening, we may want to make sure that

 - "test_must_fail git apply"

 - There is no "foo" (or "tmp"); the input to 'git apply' is the
   only thing that could create, as you do not create symlinks as
   traps before running 'git apply', so this will catch the 'make it
   incremental and then break the do-not-follow logic'.

 - There is no "../bar" (or "../foo").

The second one is missing from your tests, I think, and it would be a very good addition, even on a platform with SYMLINKS prerequisite satisfied. The future change might be trying to make it incremental and impelent do-not-follow logic by observing what is in the filesystem, and we do want to catch such a broken implementation.