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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.