Re: [PATCH] config: teach "git config --file -" to read from the standard input

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

Jeff King <peff@peff.net> writes:

>> +	} else {
>> +		if (cf->name)
>> +			return error("bad config file line %d in %s",
>> +					cf->linenr, cf->name);
>> +		else
>> +			return error("bad config file line %d", cf->linenr);
>> +	}
>
> I think I preferred the earlier version where you had "<stdin>" in the
> name field, and this hunk could just go away. I know you switched it to
> NULL here to avoid making bogus relative filenames in includes.

Exactly the same comment here.  I really like the way how this
series becomes more polished every time we see it.

> But that would be pretty straightforward to fix by splitting the "name"
> field into an additional "path" field. It looks like "git config --blob"
> has the same problem (it will try relative lookups in the filesystem,
> even though that is nonsensical from the blob). So we could fix the
> existing bug at the same time. :)
>
> Perhaps this could go at the start of your series?

Sounds like the right thing to do.

Thanks.

> -- >8 --
> Subject: config: disallow relative include paths from blobs
>
> When we see a relative config include like:
>
>   [include]
>   path = foo
>
> we make it relative to the containing directory of the file
> that contains the snippet. This makes no sense for config
> read from a blob, as it is not on the filesystem.  Something
> like "HEAD:some/path" could have a relative path within the
> tree, but:
>
>   1. It would not be part of include.path, which explicitly
>      refers to the filesystem.
>
>   2. It would need different parsing rules anyway to
>      determine that it is a tree path.
>
> The current code just uses the "name" field, which is wrong.
> Let's split that into "name" and "path" fields, use the
> latter for relative includes, and fill in only the former
> for blobs.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I don't think we considered includes at all when adding --blob. I cannot
> think of any good reason to have even an absolute filesystem include
> from a blob. And I wonder if it is possible to cause security mischief
> by convincing git to look at /etc/passwd or similar (it would not parse,
> of course, but you might be able to glean information from the error
> messages or something).
>
> It's probably OK, though, because you would generally not read real
> config from an untrusted source (there are many bad things they could
> set), and other code which uses the config reader explicitly does not
> turn on includes.
>
>  config.c                  | 10 ++++++----
>  t/t1305-config-include.sh | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index d969a5a..b295310 100644
> --- a/config.c
> +++ b/config.c
> @@ -21,6 +21,7 @@ struct config_source {
>  		} buf;
>  	} u;
>  	const char *name;
> +	const char *path;
>  	int die_on_error;
>  	int linenr;
>  	int eof;
> @@ -97,12 +98,12 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>  	if (!is_absolute_path(path)) {
>  		char *slash;
>  
> -		if (!cf || !cf->name)
> +		if (!cf || !cf->path)
>  			return error("relative config includes must come from files");
>  
> -		slash = find_last_dir_sep(cf->name);
> +		slash = find_last_dir_sep(cf->path);
>  		if (slash)
> -			strbuf_add(&buf, cf->name, slash - cf->name + 1);
> +			strbuf_add(&buf, cf->path, slash - cf->path + 1);
>  		strbuf_addstr(&buf, path);
>  		path = buf.buf;
>  	}
> @@ -1040,7 +1041,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
>  		struct config_source top;
>  
>  		top.u.file = f;
> -		top.name = filename;
> +		top.name = top.path = filename;
>  		top.die_on_error = 1;
>  		top.do_fgetc = config_file_fgetc;
>  		top.do_ungetc = config_file_ungetc;
> @@ -1062,6 +1063,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>  	top.u.buf.len = len;
>  	top.u.buf.pos = 0;
>  	top.name = name;
> +	top.path = NULL;
>  	top.die_on_error = 0;
>  	top.do_fgetc = config_buf_fgetc;
>  	top.do_ungetc = config_buf_ungetc;
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index a707076..6edd38b 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -122,6 +122,22 @@ test_expect_success 'relative includes from command line fail' '
>  	test_must_fail git -c include.path=one config test.one
>  '
>  
> +test_expect_success 'absolute includes from blobs work' '
> +	echo "[test]one = 1" >one &&
> +	echo "[include]path=$(pwd)/one" >blob &&
> +	blob=$(git hash-object -w blob) &&
> +	echo 1 >expect &&
> +	git config --blob=$blob test.one >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'relative includes from blobs fail' '
> +	echo "[test]one = 1" >one &&
> +	echo "[include]path=one" >blob &&
> +	blob=$(git hash-object -w blob) &&
> +	test_must_fail git config --blob=$blob test.one
> +'
> +
>  test_expect_success 'include cycles are detected' '
>  	cat >.gitconfig <<-\EOF &&
>  	[test]value = gitconfig