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

Jeff King [off-list ref] writes:

On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

quoted

+test_expect_failure 'symlink escape via ..' '
+	{
+		mkpatch_symlink tmp .. &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing ../foo
+'

By the way, does this patch (and the other symlink-escape ones) need to
be marked with the SYMLINKS prereq? For a pure-index application, it
should work anywhere, but I have a feeling that this "git apply patch"
may try to write the symlink to the filesystem, fail, and report failure
for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
test on, though.

We check the links to be created by the patch itself in-core before
going to the filesystem, and the symbolic links you are creating
using mkpatch_symlink should be caught before we invoke symlink(2),
I think.

In other words, this series attempts to stick to the "verify
everything in-core before deciding that it is OK to touch the
working tree or the index".

A few new tests in t4122 do try to see that the command is not
fooled by existihng symbolic links on the filesystem and they need
to be marked with SYMLINKS prerequisite.

On Tue, Feb 03, 2015 at 12:23:28PM -0800, Junio C Hamano wrote:

quoted

By the way, does this patch (and the other symlink-escape ones) need to
be marked with the SYMLINKS prereq? For a pure-index application, it
should work anywhere, but I have a feeling that this "git apply patch"
may try to write the symlink to the filesystem, fail, and report failure
for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
test on, though.

We check the links to be created by the patch itself in-core before
going to the filesystem, and the symbolic links you are creating
using mkpatch_symlink should be caught before we invoke symlink(2),
I think.

In other words, this series attempts to stick to the "verify
everything in-core before deciding that it is OK to touch the
working tree or the index".

Right, I do not think these tests will _fail_ when the filesystem does
not support symlinks. But nor are they actually testing anything
interesting. They would pass on such a system even without your patch,
as we would fail to apply even the symlink creation part of the patch.

I can live with leaving them unmarked, though. It gets the code
exercised on more systems, which gives a slightly higher chance of
catching some other unexpected breakage.

-Peff