Re: [PATCH 1/8] t3011: demonstrate directory traversal failures

[Denton Liu <liu.denton@gmail.com>]

Hi Elijah,

On Mon, Dec 09, 2019 at 08:47:38PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Add several tests demonstrating directory traversal failures of various
> sorts in dir.c (and one similar looking test that turns out to be a
> git_fnmatch bug).  A lot of these tests look like near duplicates of
> each other, but an optimization path in dir.c to pre-descend into a
> common prefix and the specialized treatment of trailing slashes in dir.c
> mean the tiny differences are sometimes important and potentially cause
> different codepaths to be explored.
> 
> Of the 7 failing tests, 2 are new to git-2.24.0 (tweaked by side effects
> of the en/clean-nested-with-ignored-topic); the other 5 also failed
> under git-2.23.0 and earlier.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  ...common-prefixes-and-directory-traversal.sh | 193 ++++++++++++++++++
>  1 file changed, 193 insertions(+)
>  create mode 100755 t/t3011-common-prefixes-and-directory-traversal.sh
> 
> diff --git a/t/t3011-common-prefixes-and-directory-traversal.sh b/t/t3011-common-prefixes-and-directory-traversal.sh
> new file mode 100755
> index 0000000000..773d6038d1
> --- /dev/null
> +++ b/t/t3011-common-prefixes-and-directory-traversal.sh
> @@ -0,0 +1,193 @@
> +#!/bin/sh
> +
> +test_description='directory traversal handling, especially with common prefixes'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	test_commit hello &&
> +
> +	>empty &&
> +	mkdir untracked_dir &&
> +	>untracked_dir/empty &&
> +	git init untracked_repo &&
> +	>untracked_repo/empty &&
> +
> +	echo ignored >.gitignore &&
> +	echo an_ignored_dir/ >>.gitignore &&

Could we just do

	cat <<-EOF >expect
	ignored
	an_ignored_dir/
	EOF

instead? Alternatively, we can use test_write_lines() but since we have
here-doc usages below already, let's keep using here-docs here to be
consistent. Same comment applies to anywhere in this patch where we have
a >>.

> +	mkdir an_ignored_dir &&
> +	mkdir an_untracked_dir &&
> +	>an_ignored_dir/ignored &&
> +	>an_ignored_dir/untracked &&
> +	>an_untracked_dir/ignored &&
> +	>an_untracked_dir/untracked
> +'
> +
> +test_expect_success 'git ls-files -o shows the right entries' '
> +	cat <<-EOF >expect &&
> +	.gitignore
> +	actual
> +	an_ignored_dir/ignored
> +	an_ignored_dir/untracked
> +	an_untracked_dir/ignored
> +	an_untracked_dir/untracked
> +	empty
> +	expect
> +	untracked_dir/empty
> +	untracked_repo/
> +	EOF
> +	git ls-files -o >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o --exclude-standard shows the right entries' '
> +	cat <<-EOF >expect &&
> +	.gitignore
> +	actual
> +	an_untracked_dir/untracked
> +	empty
> +	expect
> +	untracked_dir/empty
> +	untracked_repo/
> +	EOF
> +	git ls-files -o --exclude-standard >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o untracked_dir recurses' '
> +	echo untracked_dir/empty >expect &&
> +	git ls-files -o untracked_dir >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o untracked_dir/ recurses' '
> +	echo untracked_dir/empty >expect &&
> +	git ls-files -o untracked_dir/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o --directory untracked_dir does not recurse' '
> +	echo untracked_dir/ >expect &&
> +	git ls-files -o --directory untracked_dir >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git ls-files -o --directory untracked_dir/ does not recurse' '
> +	echo untracked_dir/ >expect &&
> +	git ls-files -o --directory untracked_dir/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o untracked_repo does not recurse' '
> +	echo untracked_repo/ >expect &&
> +	git ls-files -o untracked_repo >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git ls-files -o untracked_repo/ does not recurse' '
> +	echo untracked_repo/ >expect &&
> +	git ls-files -o untracked_repo/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git ls-files -o untracked_dir untracked_repo recurses into untracked_dir only' '
> +	echo untracked_dir/empty >expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o untracked_dir untracked_repo >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o untracked_dir/ untracked_repo/ recurses into untracked_dir only' '
> +	echo untracked_dir/empty >expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o untracked_dir/ untracked_repo/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'git ls-files -o --directory untracked_dir untracked_repo does not recurse' '
> +	echo untracked_dir/ >expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o --directory untracked_dir untracked_repo >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o --directory untracked_dir/ untracked_repo/ does not recurse' '
> +	echo untracked_dir/ >expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o --directory untracked_dir/ untracked_repo/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o .git shows nothing' '
> +	>expect &&
> +	git ls-files -o .git >actual &&
> +	test_cmp expect actual
> +'

Could we use test_must_be_empty here instead?

> +
> +test_expect_failure 'git ls-files -o .git/ shows nothing' '
> +	>expect &&
> +	git ls-files -o .git/ >actual &&
> +	test_cmp expect actual
> +'

test_must_be_empty?

Thanks,

Denton

> +
> +test_expect_success FUNNYNAMES 'git ls-files -o untracked_* recurses appropriately' '
> +	mkdir "untracked_*" &&
> +	>"untracked_*/empty" &&
> +
> +	echo "untracked_*/empty" >expect &&
> +	echo untracked_dir/empty >>expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o "untracked_*" >actual &&
> +	test_cmp expect actual
> +'
> +
> +# It turns out fill_directory returns the right paths, but ls-files' post-call
> +# filtering in show_dir_entry() via calling dir_path_match() which ends up
> +# in git_fnmatch() has logic for PATHSPEC_ONESTAR that assumes the pathspec
> +# must match the full path; it doesn't check it for matching a leading
> +# directory.
> +test_expect_failure FUNNYNAMES 'git ls-files -o untracked_*/ recurses appropriately' '
> +	echo "untracked_*/empty" >expect &&
> +	echo untracked_dir/empty >>expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o "untracked_*/" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success FUNNYNAMES 'git ls-files -o --directory untracked_* does not recurse' '
> +	echo "untracked_*/" >expect &&
> +	echo untracked_dir/ >>expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o --directory "untracked_*" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success FUNNYNAMES 'git ls-files -o --directory untracked_*/ does not recurse' '
> +	echo "untracked_*/" >expect &&
> +	echo untracked_dir/ >>expect &&
> +	echo untracked_repo/ >>expect &&
> +	git ls-files -o --directory "untracked_*/" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git ls-files -o consistent between one or two dirs' '
> +	git ls-files -o --exclude-standard an_ignored_dir/ an_untracked_dir/ >tmp &&
> +	! grep ^an_ignored_dir/ tmp >expect &&
> +	git ls-files -o --exclude-standard an_ignored_dir/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +# ls-files doesn't have a way to request showing both untracked and ignored
> +# files at the same time, so use `git status --ignored`
> +test_expect_failure 'git status --ignored shows same files under dir with or without pathspec' '
> +	cat <<-EOF >expect &&
> +	?? an_untracked_dir/
> +	!! an_untracked_dir/ignored
> +	EOF
> +	git status --porcelain --ignored >output &&
> +	grep an_untracked_dir output >expect &&
> +	git status --porcelain --ignored an_untracked_dir/ >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> -- 
> gitgitgadget
>