[PATCH 0/3] Updates to the safe.directory config option

[PATCH 0/3] Updates to the safe.directory config option

[Derrick Stolee via GitGitGadget]

Here is a very fast response to the security release yesterday.

The second patch here is an adaptation from a contributor who created a pull
request against git/git [1]. I augmented the patch with a test (the test
infrastructure is added in patch 1).

The third patch is a change to the safe.directory config option to include a
possible "*" value to completely opt-out of the check. This will be
particularly helpful for cases where users run Git commands within a
container. This container workflow always runs as a different user than the
host, but also the container does not have access to the host's system or
global config files. It's also helpful for users who don't want to set the
config for a large number of shared repositories [2].

Thanks, -Stolee

[1] https://github.com/git/git/pull/1235 [2]
https://github.com/git-for-windows/git/issues/3787 [3]
https://github.com/desktop/desktop/issues/14336

Derrick Stolee (2):
  t0033: add tests for safe.directory
  setup: opt-out of check with safe.directory=*

Matheus Valadares (1):
  setup: fix safe.directory key not being checked

 Documentation/config/safe.txt |  7 +++++
 setup.c                       | 12 ++++++---
 t/t0033-safe-directory.sh     | 49 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100755 t/t0033-safe-directory.sh


base-commit: 11cfe552610386954886543f5de87dcc49ad5735
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1215
-- 
gitgitgadget

[PATCH 1/3] t0033: add tests for safe.directory

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

It is difficult to change the ownership on a directory in our test
suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
variable to trick Git into thinking we are in a differently-owned
directory. This allows us to test that the config is parsed correctly.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 setup.c                   |  3 ++-
 t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100755 t/t0033-safe-directory.sh

diff --git a/setup.c b/setup.c
index c8f67bfed52..f54f449008a 100644
--- a/setup.c
+++ b/setup.c
@@ -1119,7 +1119,8 @@ static int ensure_valid_ownership(const char *path)
 {
 	struct safe_directory_data data = { .path = path };
 
-	if (is_path_owned_by_current_user(path))
+	if (is_path_owned_by_current_user(path) &&
+	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
 		return 1;
 
 	read_very_early_config(safe_directory_cb, &data);
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
new file mode 100755
index 00000000000..9380ff3d017
--- /dev/null
+++ b/t/t0033-safe-directory.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='verify safe.directory checks'
+
+. ./test-lib.sh
+
+GIT_TEST_ASSUME_DIFFERENT_OWNER=1
+export GIT_TEST_ASSUME_DIFFERENT_OWNER
+
+expect_rejected_dir () {
+	test_must_fail git status 2>err &&
+	grep "safe.directory" err
+}
+
+test_expect_success 'safe.directory is not set' '
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory does not match' '
+	git config --global safe.directory bogus &&
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory matches' '
+	git config --global --add safe.directory "$(pwd)" &&
+	git status
+'
+
+test_expect_success 'safe.directory matches, but is reset' '
+	git config --global --add safe.directory "" &&
+	expect_rejected_dir
+'
+
+test_done
-- 
gitgitgadget

[PATCH 2/3] setup: fix safe.directory key not being checked

[Matheus Valadares via GitGitGadget]

From: Matheus Valadares <me@m28.io>

It seems that nothing is ever checking to make sure the safe directories
in the configs actually have the key safe.directory, so some unrelated
config that has a value with a certain directory would also make it a
safe directory.

Signed-off-by: Matheus Valadares <me@m28.io>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 setup.c                   | 3 +++
 t/t0033-safe-directory.sh | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/setup.c b/setup.c
index f54f449008a..a995c359c32 100644
--- a/setup.c
+++ b/setup.c
@@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
 {
 	struct safe_directory_data *data = d;
 
+	if (strcmp(key, "safe.directory"))
+		return 0;
+
 	if (!value || !*value)
 		data->is_safe = 0;
 	else {
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 9380ff3d017..6f33c0dfefa 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '
 	expect_rejected_dir
 '
 
+test_expect_success 'path exist as different key' '
+	git config --global foo.bar "$(pwd)" &&
+	expect_rejected_dir
+'
+
 test_expect_success 'safe.directory matches' '
 	git config --global --add safe.directory "$(pwd)" &&
 	git status
-- 
gitgitgadget

[PATCH 3/3] setup: opt-out of check with safe.directory=*

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

With the addition of the safe.directory in 8959555ce
(setup_git_directory(): add an owner check for the top-level directory,
2022-03-02) released in v2.35.2, we are receiving feedback from a
variety of users about the feature.

Some users have a very large list of shared repositories and find it
cumbersome to add this config for every one of them.

In a more difficult case, certain workflows involve running Git commands
within containers. The container boundary prevents any global or system
config from communicating `safe.directory` values from the host into the
container. Further, the container almost always runs as a different user
than the owner of the directory in the host.

To simplify the reactions necessary for these users, extend the
definition of the safe.directory config value to include a possible '*'
value. This value implies that all directories are safe, providing a
single setting to opt-out of this protection.

Note that an empty assignment of safe.directory clears all previous
values, and this is already the case with the "if (!value || !*value)"
condition.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/config/safe.txt |  7 +++++++
 setup.c                       |  6 ++++--
 t/t0033-safe-directory.sh     | 10 ++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
index 63597b2df8f..6d764fe0ccf 100644
--- a/Documentation/config/safe.txt
+++ b/Documentation/config/safe.txt
@@ -19,3 +19,10 @@ line option `-c safe.directory=<path>`.
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a
 path relative to Git's (runtime) prefix.
++
+To completely opt-out of this security check, set `safe.directory` to the
+string `*`. This will allow all repositories to be treated as if their
+directory was listed in the `safe.directory` list. If `safe.directory=*`
+is set in system config and you want to re-enable this protection, then
+initialize your list with an empty value before listing the repositories
+that you deem safe.
diff --git a/setup.c b/setup.c
index a995c359c32..a42b21307f7 100644
--- a/setup.c
+++ b/setup.c
@@ -1103,9 +1103,11 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value)
+	if (!value || !*value) {
 		data->is_safe = 0;
-	else {
+	} else if (!strcmp(value, "*")) {
+		data->is_safe = 1;
+	} else {
 		const char *interpolated = NULL;
 
 		if (!git_config_pathname(&interpolated, key, value) &&
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 6f33c0dfefa..239d93f4d21 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '
 	expect_rejected_dir
 '
 
+test_expect_success 'safe.directory=*' '
+	git config --global --add safe.directory "*" &&
+	git status
+'
+
+test_expect_success 'safe.directory=*, but is reset' '
+	git config --global --add safe.directory "" &&
+	expect_rejected_dir
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 0/3] Updates to the safe.directory config option

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Here is a very fast response to the security release yesterday.

Wow.  While I were down the whole day yesterday after sending the
release announcement, it seems a lot have happened X-<.  Does your
"a very fast" expect only "wow, thanks for a fast reponse", or does
it also expect "ok, we'll take a deep look with a spoonful of salt
as it was prepared in haste"?

> The second patch here is an adaptation from a contributor who created a pull
> request against git/git [1]. I augmented the patch with a test (the test
> infrastructure is added in patch 1).
>
> The third patch is a change to the safe.directory config option to include a
> possible "*" value to completely opt-out of the check. This will be
> particularly helpful for cases where users run Git commands within a
> container. This container workflow always runs as a different user than the
> host, but also the container does not have access to the host's system or
> global config files. It's also helpful for users who don't want to set the
> config for a large number of shared repositories [2].

Let me take a look how well these integrate into the maintenance
tracks.

I would appreciate something that is targetted and narrow that can
be applied to the oldest maintenance track (2.30.3) and then merged
upwards, plus niceties on top that does not necessarily have to
apply to the oldest ones if the surrounding code or tests were
changed more recently.

Thanks.

> Thanks, -Stolee
>
> [1] https://github.com/git/git/pull/1235 [2]
> https://github.com/git-for-windows/git/issues/3787 [3]
> https://github.com/desktop/desktop/issues/14336
>
> Derrick Stolee (2):
>   t0033: add tests for safe.directory
>   setup: opt-out of check with safe.directory=*
>
> Matheus Valadares (1):
>   setup: fix safe.directory key not being checked
>
>  Documentation/config/safe.txt |  7 +++++
>  setup.c                       | 12 ++++++---
>  t/t0033-safe-directory.sh     | 49 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 3 deletions(-)
>  create mode 100755 t/t0033-safe-directory.sh
>
>
> base-commit: 11cfe552610386954886543f5de87dcc49ad5735
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1215

Re: [PATCH 1/3] t0033: add tests for safe.directory

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> It is difficult to change the ownership on a directory in our test
> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
> variable to trick Git into thinking we are in a differently-owned
> directory. This allows us to test that the config is parsed correctly.

OK.

> -	if (is_path_owned_by_current_user(path))
> +	if (is_path_owned_by_current_user(path) &&
> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>  		return 1;

Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
real logic kicks in, I wonder?

> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> new file mode 100755
> index 00000000000..9380ff3d017
> --- /dev/null
> +++ b/t/t0033-safe-directory.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='verify safe.directory checks'
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> +
> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +...
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&
> +	git status
> +'

Just double checking, as I know you are much closer to the affected
platform than I'd ever be ;-) but is the use of $(pwd) safe and
correct here?

I always get confused between $(pwd) and $PWD, which does not make
any difference on platforms I have access to, but makes difference
to hurt Windows users.

> +test_expect_success 'safe.directory matches, but is reset' '
> +	git config --global --add safe.directory "" &&
> +	expect_rejected_dir
> +'
> +
> +test_done

Thanks.  This step should apply to maint-2.30 cleanly, I would
think.

Re: [PATCH 0/3] Updates to the safe.directory config option

[Derrick Stolee]

On 4/13/2022 12:15 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> Here is a very fast response to the security release yesterday.
> 
> Wow.  While I were down the whole day yesterday after sending the
> release announcement, it seems a lot have happened X-<.  Does your
> "a very fast" expect only "wow, thanks for a fast reponse", or does
> it also expect "ok, we'll take a deep look with a spoonful of salt
> as it was prepared in haste"?

I tried to do my due diligence here, but I will admit to some amount
of haste being applied due to the many distinct sources that have
motivated the change.

>> The second patch here is an adaptation from a contributor who created a pull
>> request against git/git [1]. I augmented the patch with a test (the test
>> infrastructure is added in patch 1).
>>
>> The third patch is a change to the safe.directory config option to include a
>> possible "*" value to completely opt-out of the check. This will be
>> particularly helpful for cases where users run Git commands within a
>> container. This container workflow always runs as a different user than the
>> host, but also the container does not have access to the host's system or
>> global config files. It's also helpful for users who don't want to set the
>> config for a large number of shared repositories [2].
> 
> Let me take a look how well these integrate into the maintenance
> tracks.
> 
> I would appreciate something that is targetted and narrow that can
> be applied to the oldest maintenance track (2.30.3) and then merged
> upwards, plus niceties on top that does not necessarily have to
> apply to the oldest ones if the surrounding code or tests were
> changed more recently.

The tests that are added are in a new test file, so hopefully
those don't collide with anything.

The changes in setup.c apply within the ensure_valid_ownership()
so should apply to any versions that have the fix.

Thanks,
-Stolee

Re: [PATCH 1/3] t0033: add tests for safe.directory

[Derrick Stolee]

On 4/13/2022 12:24 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> It is difficult to change the ownership on a directory in our test
>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
>> variable to trick Git into thinking we are in a differently-owned
>> directory. This allows us to test that the config is parsed correctly.
> 
> OK.
> 
>> -	if (is_path_owned_by_current_user(path))
>> +	if (is_path_owned_by_current_user(path) &&
>> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>>  		return 1;
> 
> Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
> real logic kicks in, I wonder?

Either order would work. I bet that checking the environment is
faster than checking the disk, so swapping the order would be prudent
here.
 
>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>> new file mode 100755
>> index 00000000000..9380ff3d017
>> --- /dev/null
>> +++ b/t/t0033-safe-directory.sh
>> @@ -0,0 +1,34 @@
>> +#!/bin/sh
>> +
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
>> +
>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}
>> +...
>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
>> +	git status
>> +'
> 
> Just double checking, as I know you are much closer to the affected
> platform than I'd ever be ;-) but is the use of $(pwd) safe and
> correct here?
> 
> I always get confused between $(pwd) and $PWD, which does not make
> any difference on platforms I have access to, but makes difference
> to hurt Windows users.

These tests pass CI on Windows. I've had issues before using $PWD,
thinking back to tests in t7900-maintenance.sh that use $(pwd)
instead.

Thanks,
-Stolee

Re: [PATCH 2/3] setup: fix safe.directory key not being checked

[Junio C Hamano]

"Matheus Valadares via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Matheus Valadares <me@m28.io>
>
> It seems that nothing is ever checking to make sure the safe directories
> in the configs actually have the key safe.directory, so some unrelated
> config that has a value with a certain directory would also make it a
> safe directory.

Good finding, and the fix is straight-forward and obviously correct.
Thanks.

> Signed-off-by: Matheus Valadares <me@m28.io>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  setup.c                   | 3 +++
>  t/t0033-safe-directory.sh | 5 +++++
>  2 files changed, 8 insertions(+)

> diff --git a/setup.c b/setup.c
> index f54f449008a..a995c359c32 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
>  {
>  	struct safe_directory_data *data = d;
>  
> +	if (strcmp(key, "safe.directory"))
> +		return 0;
> +
>  	if (!value || !*value)
>  		data->is_safe = 0;
>  	else {
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 9380ff3d017..6f33c0dfefa 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'path exist as different key' '
> +	git config --global foo.bar "$(pwd)" &&
> +	expect_rejected_dir
> +'
> +
>  test_expect_success 'safe.directory matches' '
>  	git config --global --add safe.directory "$(pwd)" &&
>  	git status

Re: [PATCH 3/3] setup: opt-out of check with safe.directory=*

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> To simplify the reactions necessary for these users, extend the
> definition of the safe.directory config value to include a possible '*'
> value. This value implies that all directories are safe, providing a
> single setting to opt-out of this protection.

OK.  During the development of the original fix, we discussed if a
more flexible mechanism, like allowing globs, but ended up with the
simplest and easiest to explain option, with the expectation that we
may want to loosen it later as necessary.  And this is certainly
what we would have expected to add.

> Note that an empty assignment of safe.directory clears all previous
> values, and this is already the case with the "if (!value || !*value)"
> condition.

OK.

>  	if (strcmp(key, "safe.directory"))
>  		return 0;
>  
> -	if (!value || !*value)
> +	if (!value || !*value) {
>  		data->is_safe = 0;
> -	else {
> +	} else if (!strcmp(value, "*")) {
> +		data->is_safe = 1;
> +	} else {
>  		const char *interpolated = NULL;
>  
>  		if (!git_config_pathname(&interpolated, key, value) &&
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh

OK.

> index 6f33c0dfefa..239d93f4d21 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'safe.directory=*' '
> +	git config --global --add safe.directory "*" &&
> +	git status
> +'
> +
> +test_expect_success 'safe.directory=*, but is reset' '
> +	git config --global --add safe.directory "" &&
> +	expect_rejected_dir
> +'

Thanks.

Re: [PATCH 0/3] Updates to the safe.directory config option

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> I tried to do my due diligence here, but I will admit to some amount
> of haste being applied due to the many distinct sources that have
> motivated the change.

OK.  Thanks for a fast response to a brown-paper-bag bug or two.

> The tests that are added are in a new test file, so hopefully
> those don't collide with anything.
>
> The changes in setup.c apply within the ensure_valid_ownership()
> so should apply to any versions that have the fix.

Yup, I was more worried about newer test helpers and API functions
that did not exist in the older code base, but after reading the
patches through, I think these are all from battle tested and solid
part that applies to all relevant maintenance tracks.

Thanks.

Re: [PATCH 1/3] t0033: add tests for safe.directory

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
>
> It is difficult to change the ownership on a directory in our test
> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
> variable to trick Git into thinking we are in a differently-owned
> directory. This allows us to test that the config is parsed correctly.

I think this is a good trade-off, but FWIW I'd think we could test also
without the git_env_bool() by having the test depend on !NOT_ROOT, then
check the owner of t/test-lib.sh, and chown to that user (i.e. the
"real" user).

But that's all sorts of more fragile than just this test variable..
> +test_description='verify safe.directory checks'
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
> +export GIT_TEST_ASSUME_DIFFERENT_OWNER

Instead of this "export" perhaps just add it in front of the "git
status"?

These tests also pass with SANITIZE=leak, so please add
TEST_PASSES_SANITIZE_LEAK=true at the top.

> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +
> +test_expect_success 'safe.directory is not set' '
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory does not match' '
> +	git config --global safe.directory bogus &&
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&

nit: $PWD instead of $(pwd)

Re: [PATCH 1/3] t0033: add tests for safe.directory

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

(just this part)

> These tests also pass with SANITIZE=leak, so please add
> TEST_PASSES_SANITIZE_LEAK=true at the top.

Derrick, please ignore the above.  It is totally outside the scope
of these patches, and they are meant to be applied on top of the
2.30 maintenance track, where TEST_PASSES_SANITIZE_LEAK=true was
irrelevant.

I do not mind adding such after the dust settles on top of 'master'
(or possibly 'maint'), but not as part of these "let's fix the screw
up in 2.30.3 and its friends" effort.

Thanks.

Re: [PATCH 1/3] t0033: add tests for safe.directory

[Derrick Stolee]

On 4/13/2022 3:16 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> It is difficult to change the ownership on a directory in our test
>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
>> variable to trick Git into thinking we are in a differently-owned
>> directory. This allows us to test that the config is parsed correctly.
> 
> I think this is a good trade-off, but FWIW I'd think we could test also
> without the git_env_bool() by having the test depend on !NOT_ROOT, then
> check the owner of t/test-lib.sh, and chown to that user (i.e. the
> "real" user).
> 
> But that's all sorts of more fragile than just this test variable..
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> 
> Instead of this "export" perhaps just add it in front of the "git
> status"?

If the only runs were in this helper below, then yes.

>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}

Later patches add more success cases that run 'git status'
as its verification that the match works. I didn't think it
was good to have this environment variable set for each of
those invocations.

This script has one purpose, and this environment variable
is required to make any of the checks work. Setting it
globally seems the best way to do that.

>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
> 
> nit: $PWD instead of $(pwd)

Historically, $PWD doesn't work properly across platforms,
so I have used $(pwd) consistently across many contributions.

Thanks,
-Stolee

Re: [PATCH 0/3] Updates to the safe.directory config option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Derrick Stolee <derrickstolee@github.com> writes:
>
>> I tried to do my due diligence here, but I will admit to some amount
>> of haste being applied due to the many distinct sources that have
>> motivated the change.
>
> OK.  Thanks for a fast response to a brown-paper-bag bug or two.

Here is a draft release notes to 2.30.4.txt; I may tag this later
tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together
with the result of merging the fixes into newer maintenance releases.

Thanks, everybody, for working on the issue.


Git v2.30.4 Release Notes (draft)
=================================

This release contains minor fix-ups for the changes that went into
Git 2.30.3, which was made to address CVE-2022-24765.

 * The code that was meant to parse the new `safe.directory`
   configuration variable was not checking what configuration
   variable was being fed to it, which has been corrected.

 * '*' can be used as the value for the `safe.directory` variable to
   signal that the user considers that any directory is safe.


Derrick Stolee (2):
      t0033: add tests for safe.directory
      setup: opt-out of check with safe.directory=*

Matheus Valadares (1):
      setup: fix safe.directory key not being checked

Re: [PATCH 0/3] Updates to the safe.directory config option

[Taylor Blau]

On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Derrick Stolee <derrickstolee@github.com> writes:
> >
> >> I tried to do my due diligence here, but I will admit to some amount
> >> of haste being applied due to the many distinct sources that have
> >> motivated the change.
> >
> > OK.  Thanks for a fast response to a brown-paper-bag bug or two.
>
> Here is a draft release notes to 2.30.4.txt; I may tag this later
> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together
> with the result of merging the fixes into newer maintenance releases.
>
> Thanks, everybody, for working on the issue.

Thanks; this all looks good to me. I took a careful look at Stolee's new
patches as well as the (elided) release notes from you below, and both
look good to me.

Don't worry about applying my Reviewed-by, but I did want to acknowledge
having looked at these.

Thanks,
Taylor

Re: [PATCH 0/3] Updates to the safe.directory config option

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Derrick Stolee <derrickstolee@github.com> writes:
>> >
>> >> I tried to do my due diligence here, but I will admit to some amount
>> >> of haste being applied due to the many distinct sources that have
>> >> motivated the change.
>> >
>> > OK.  Thanks for a fast response to a brown-paper-bag bug or two.
>>
>> Here is a draft release notes to 2.30.4.txt; I may tag this later
>> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together
>> with the result of merging the fixes into newer maintenance releases.
>>
>> Thanks, everybody, for working on the issue.
>
> Thanks; this all looks good to me. I took a careful look at Stolee's new
> patches as well as the (elided) release notes from you below, and both
> look good to me.
>
> Don't worry about applying my Reviewed-by, but I did want to acknowledge
> having looked at these.

Thanks for giving an extra set of eyes.

[PATCH 0/3] t0033-safe-directory: test improvements and a documentation clarification

[SZEDER Gábor]

Add coverage for the cases when 'safe.directory' is ignored, a minor
test improvement, and a small documentation clarification.

SZEDER Gábor (3):
  t0033-safe-directory: check the error message without matching the
    trash dir
  t0033-safe-directory: check when 'safe.directory' is ignored
  safe.directory: document and check that it's ignored in the
    environment

 Documentation/config/safe.txt |  4 ++--
 t/t0033-safe-directory.sh     | 30 +++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.36.0.676.gf39b21ed98

[PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

[SZEDER Gábor]

Since 8959555cee (setup_git_directory(): add an owner check for the
top-level directory, 2022-03-02) when git finds itself in a repository
owned by someone else, it aborts with a "fatal: unsafe repository
(<repo path>)" error message and an advice about how to set the
'safe.directory' config variable to mark that repository as safe.
't0033-safe-directory.sh' contains tests that check that this feature
and handling said config work as intended.  To ensure that git dies
for the right reason, several of those tests check that its standard
error contains the name of that config variable, but:

  - it only appears in the advice part, not in the actual error
    message.

  - it is interpreted as a regexp by 'grep', so, because of the dot,
    it matches the name of the test script and the path of the trash
    directory as well.  Consequently, these tests could be fooled by
    any error message that would happen to include the path of the
    test repository.

Tighten these checks to look for "unsafe repository" instead.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0033-safe-directory.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 239d93f4d2..6f9680e8b0 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER
 
 expect_rejected_dir () {
 	test_must_fail git status 2>err &&
-	grep "safe.directory" err
+	grep "unsafe repository" err
 }
 
 test_expect_success 'safe.directory is not set' '
-- 
2.36.0.676.gf39b21ed98

[PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[SZEDER Gábor]

According to the documentation 'safe.directory' "is only respected
when specified in a system or global config, not when it is specified
in a repository config or via the command line option -c
safe.directory=<path>".

Add tests to check that 'safe.directory' in the repository config or
on the command line is indeed ignored.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0033-safe-directory.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 6f9680e8b0..82dac0eb93 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
 	expect_rejected_dir
 '
 
+test_expect_success 'ignoring safe.directory on the command line' '
+	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
+	grep "unsafe repository" err
+'
+
+test_expect_success 'ignoring safe.directory in repo config' '
+	(
+		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config safe.directory "$(pwd)"
+	) &&
+	expect_rejected_dir
+'
+
 test_expect_success 'safe.directory does not match' '
 	git config --global safe.directory bogus &&
 	expect_rejected_dir
-- 
2.36.0.676.gf39b21ed98

[PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[SZEDER Gábor]

The description of 'safe.directory' mentions that it's respected in
the system and global configs, and ignored in the repository config
and on the command line, but it doesn't mention whether it's respected
or ignored when specified via environment variables (nor does the
commit message adding 'safe.directory' [1]).

Clarify that 'safe.directory' is ignored when specified in the
environment, and add tests to make sure that it remains so.

[1] 8959555cee (setup_git_directory(): add an owner check for the
                top-level directory, 2022-03-02)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/config/safe.txt |  4 ++--
 t/t0033-safe-directory.sh     | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
index 6d764fe0cc..ae0e2e3bdb 100644
--- a/Documentation/config/safe.txt
+++ b/Documentation/config/safe.txt
@@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
 `safe.directory` entry with an empty value.
 +
 This config setting is only respected when specified in a system or global
-config, not when it is specified in a repository config or via the command
-line option `-c safe.directory=<path>`.
+config, not when it is specified in a repository config, via the command
+line option `-c safe.directory=<path>`, or in environment variables.
 +
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 82dac0eb93..238b25f91a 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '
 	grep "unsafe repository" err
 '
 
+test_expect_success 'ignoring safe.directory in the environment' '
+	test_must_fail env GIT_CONFIG_COUNT=1 \
+		GIT_CONFIG_KEY_0="safe.directory" \
+		GIT_CONFIG_VALUE_0="$(pwd)" \
+		git status 2>err &&
+	grep "unsafe repository" err
+'
+
+test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
+	test_must_fail env \
+		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
+		git status 2>err &&
+	grep "unsafe repository" err
+'
+
 test_expect_success 'ignoring safe.directory in repo config' '
 	(
 		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
-- 
2.36.0.676.gf39b21ed98

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> According to the documentation 'safe.directory' "is only respected
> when specified in a system or global config, not when it is specified
> in a repository config or via the command line option -c
> safe.directory=<path>".
>
> Add tests to check that 'safe.directory' in the repository config or
> on the command line is indeed ignored.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t0033-safe-directory.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 6f9680e8b0..82dac0eb93 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'ignoring safe.directory on the command line' '
> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
> +test_expect_success 'ignoring safe.directory in repo config' '
> +	(
> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config safe.directory "$(pwd)"
> +	) &&
> +	expect_rejected_dir
> +'

I am debating myself if we want to remove the in-repository
safe.directory configuration setting after this test piece is done,
with test_when_finished.  We just made sure, with this test, that
having the variable does not affect anything, so the subsequent
tests should not care hence it is probably OK.  On the other hand,
to make sure those settings they make (e.g. setting it globally is
what the next test we can see below does) is what affects their
outcome, it removes one more thing we need to worry about if we
clean after ourselves.  I dunno.

Other than that, looking good so far; both [1/3] and [2/3] look good.

>  test_expect_success 'safe.directory does not match' '
>  	git config --global safe.directory bogus &&
>  	expect_rejected_dir

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> The description of 'safe.directory' mentions that it's respected in
> the system and global configs, and ignored in the repository config
> and on the command line, but it doesn't mention whether it's respected
> or ignored when specified via environment variables (nor does the
> commit message adding 'safe.directory' [1]).

If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
$PATH does), that would have been absolutely necessary to document
how it works, but GIT_CONFIG_* is merely an implementation detail of
how "git -c var=val" works and I am not sure if it is even a good
idea to hardcode how they happen to work like these tests.  The only
thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
used internally by the implementation and they should not muck with
it, no?

I am moderately negative about this change.

> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 82dac0eb93..238b25f91a 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '
>  	grep "unsafe repository" err
>  '
>  
> +test_expect_success 'ignoring safe.directory in the environment' '
> +	test_must_fail env GIT_CONFIG_COUNT=1 \
> +		GIT_CONFIG_KEY_0="safe.directory" \
> +		GIT_CONFIG_VALUE_0="$(pwd)" \
> +		git status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
> +test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
> +	test_must_fail env \
> +		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
> +		git status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
>  test_expect_success 'ignoring safe.directory in repo config' '
>  	(
>  		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> $PATH does), that would have been absolutely necessary to document
> how it works, but GIT_CONFIG_* is merely an implementation detail of
> how "git -c var=val" works and I am not sure if it is even a good
> idea to hardcode how they happen to work like these tests.  The only
> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> used internally by the implementation and they should not muck with
> it, no?

I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
scripts, but then ...

> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> index 6d764fe0cc..ae0e2e3bdb 100644
> --- a/Documentation/config/safe.txt
> +++ b/Documentation/config/safe.txt
> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
>  `safe.directory` entry with an empty value.
>  +
>  This config setting is only respected when specified in a system or global
> -config, not when it is specified in a repository config or via the command
> -line option `-c safe.directory=<path>`.
> +config, not when it is specified in a repository config, via the command
> +line option `-c safe.directory=<path>`, or in environment variables.

... this part must clarify what environment variables it is talking
about.

    ... via the command line option `-c safe.directory=<path>`, or
    via the GIT_CONFIG_{KEY,VALUE} mechanism.

or something, perhaps.  I actually do think it is a useful addition
to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
ignored.

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Derrick Stolee]

On 4/27/2022 4:37 PM, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
>> According to the documentation 'safe.directory' "is only respected
>> when specified in a system or global config, not when it is specified
>> in a repository config or via the command line option -c
>> safe.directory=<path>".
>>
>> Add tests to check that 'safe.directory' in the repository config or
>> on the command line is indeed ignored.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  t/t0033-safe-directory.sh | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>> index 6f9680e8b0..82dac0eb93 100755
>> --- a/t/t0033-safe-directory.sh
>> +++ b/t/t0033-safe-directory.sh
>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>>  	expect_rejected_dir
>>  '
>>  
>> +test_expect_success 'ignoring safe.directory on the command line' '
>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
>> +	grep "unsafe repository" err
>> +'
>> +
>> +test_expect_success 'ignoring safe.directory in repo config' '
>> +	(
>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>> +		git config safe.directory "$(pwd)"
>> +	) &&
>> +	expect_rejected_dir
>> +'
> 
> I am debating myself if we want to remove the in-repository
> safe.directory configuration setting after this test piece is done,
> with test_when_finished.  We just made sure, with this test, that
> having the variable does not affect anything, so the subsequent
> tests should not care hence it is probably OK.  On the other hand,
> to make sure those settings they make (e.g. setting it globally is
> what the next test we can see below does) is what affects their
> outcome, it removes one more thing we need to worry about if we
> clean after ourselves.  I dunno.

test_config would do the same, right? I think it automatically
does the test_when_finished for us.

Thanks,
-Stolee

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[Derrick Stolee]

On 4/27/2022 4:49 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
>> $PATH does), that would have been absolutely necessary to document
>> how it works, but GIT_CONFIG_* is merely an implementation detail of
>> how "git -c var=val" works and I am not sure if it is even a good
>> idea to hardcode how they happen to work like these tests.  The only
>> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
>> used internally by the implementation and they should not muck with
>> it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
>> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
>> index 6d764fe0cc..ae0e2e3bdb 100644
>> --- a/Documentation/config/safe.txt
>> +++ b/Documentation/config/safe.txt
>> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
>>  `safe.directory` entry with an empty value.
>>  +
>>  This config setting is only respected when specified in a system or global
>> -config, not when it is specified in a repository config or via the command
>> -line option `-c safe.directory=<path>`.
>> +config, not when it is specified in a repository config, via the command
>> +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.
> 
> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.

I agree on both points.

Thanks for working on this!
-Stolee

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 4/27/2022 4:37 PM, Junio C Hamano wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>> 
>>> According to the documentation 'safe.directory' "is only respected
>>> when specified in a system or global config, not when it is specified
>>> in a repository config or via the command line option -c
>>> safe.directory=<path>".
>>>
>>> Add tests to check that 'safe.directory' in the repository config or
>>> on the command line is indeed ignored.
>>>
>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>>> ---
>>>  t/t0033-safe-directory.sh | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>>> index 6f9680e8b0..82dac0eb93 100755
>>> --- a/t/t0033-safe-directory.sh
>>> +++ b/t/t0033-safe-directory.sh
>>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>>>  	expect_rejected_dir
>>>  '
>>>  
>>> +test_expect_success 'ignoring safe.directory on the command line' '
>>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
>>> +	grep "unsafe repository" err
>>> +'
>>> +
>>> +test_expect_success 'ignoring safe.directory in repo config' '
>>> +	(
>>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>>> +		git config safe.directory "$(pwd)"
>>> +	) &&
>>> +	expect_rejected_dir
>>> +'
>> 
>> I am debating myself if we want to remove the in-repository
>> safe.directory configuration setting after this test piece is done,
>> with test_when_finished.  We just made sure, with this test, that
>> having the variable does not affect anything, so the subsequent
>> tests should not care hence it is probably OK.  On the other hand,
>> to make sure those settings they make (e.g. setting it globally is
>> what the next test we can see below does) is what affects their
>> outcome, it removes one more thing we need to worry about if we
>> clean after ourselves.  I dunno.
>
> test_config would do the same, right? I think it automatically
> does the test_when_finished for us.

I thought it (specifically, anything depends on test_when_finished)
has trouble working well from inside a subprocess?

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[SZEDER Gábor]

On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
> > On 4/27/2022 4:37 PM, Junio C Hamano wrote:
> >> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >> 
> >>> According to the documentation 'safe.directory' "is only respected
> >>> when specified in a system or global config, not when it is specified
> >>> in a repository config or via the command line option -c
> >>> safe.directory=<path>".
> >>>
> >>> Add tests to check that 'safe.directory' in the repository config or
> >>> on the command line is indeed ignored.
> >>>
> >>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> >>> ---
> >>>  t/t0033-safe-directory.sh | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> >>> index 6f9680e8b0..82dac0eb93 100755
> >>> --- a/t/t0033-safe-directory.sh
> >>> +++ b/t/t0033-safe-directory.sh
> >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
> >>>  	expect_rejected_dir
> >>>  '
> >>>  
> >>> +test_expect_success 'ignoring safe.directory on the command line' '
> >>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
> >>> +	grep "unsafe repository" err
> >>> +'
> >>> +
> >>> +test_expect_success 'ignoring safe.directory in repo config' '
> >>> +	(
> >>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> >>> +		git config safe.directory "$(pwd)"
> >>> +	) &&
> >>> +	expect_rejected_dir
> >>> +'
> >> 
> >> I am debating myself if we want to remove the in-repository
> >> safe.directory configuration setting after this test piece is done,
> >> with test_when_finished.  We just made sure, with this test, that
> >> having the variable does not affect anything, so the subsequent
> >> tests should not care hence it is probably OK.  On the other hand,
> >> to make sure those settings they make (e.g. setting it globally is
> >> what the next test we can see below does) is what affects their
> >> outcome, it removes one more thing we need to worry about if we
> >> clean after ourselves.  I dunno.
> >
> > test_config would do the same, right? I think it automatically
> > does the test_when_finished for us.
> 
> I thought it (specifically, anything depends on test_when_finished)
> has trouble working well from inside a subprocess?

Yeah, test_config doesn't work in a subshell, because modifying
the variable holding the cleanup commands won't be visible after
leaving the subshell, and the protection added in 0968f12a99
(test-lib-functions: detect test_when_finished in subshell,
2015-09-05) will kick in.  And we do need a subshell to set the
config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
config' would refuse to touch the config file.

I think something like

  test_when_finished "(
        unset GIT_TEST_ASSUME_DIFFERENT_USER &&
        git config --unset safe.directory
        )"

would work, though.

I considered cleaning up the config file, but all subsequent tests
leave behind config settings for later tests as well (in the global
config file, though).

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Derrick Stolee]

On 4/29/2022 3:06 PM, SZEDER Gábor wrote:
> On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:
>> Derrick Stolee <derrickstolee@github.com> writes:
>>> test_config would do the same, right? I think it automatically
>>> does the test_when_finished for us.
>>
>> I thought it (specifically, anything depends on test_when_finished)
>> has trouble working well from inside a subprocess?
> 
> Yeah, test_config doesn't work in a subshell, because modifying
> the variable holding the cleanup commands won't be visible after
> leaving the subshell, and the protection added in 0968f12a99
> (test-lib-functions: detect test_when_finished in subshell,
> 2015-09-05) will kick in.  And we do need a subshell to set the
> config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> config' would refuse to touch the config file.

Ah yes, of course.
 
> I think something like
> 
>   test_when_finished "(
>         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
>         git config --unset safe.directory
>         )"
> 
> would work, though.

Would it be simpler to use this?

	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory

Thanks,
-Stolee

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[SZEDER Gábor]

On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> > $PATH does), that would have been absolutely necessary to document
> > how it works, but GIT_CONFIG_* is merely an implementation detail of
> > how "git -c var=val" works and I am not sure if it is even a good
> > idea to hardcode how they happen to work like these tests.  The only
> > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> > used internally by the implementation and they should not muck with
> > it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> > index 6d764fe0cc..ae0e2e3bdb 100644
> > --- a/Documentation/config/safe.txt
> > +++ b/Documentation/config/safe.txt
> > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
> >  `safe.directory` entry with an empty value.
> >  +
> >  This config setting is only respected when specified in a system or global
> > -config, not when it is specified in a repository config or via the command
> > -line option `-c safe.directory=<path>`.
> > +config, not when it is specified in a repository config, via the command
> > +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.

Well, the proposed phrasing covers all environment variables that
affect the configuration.

It doesn't feel right to explicitly list all those variables,
including GIT_CONFIG_PARAMETERS, because that vairable is such an
implementation detail that it isn't even mentioned elsewhere in the
documentation at all.  OTOH, listing only some of the ignored
variables but omitting others doesn't feel quite right either, hence
the general "in environment variables".

> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.

Is it safe to have such an environment variable?

So, it's quite clear why 'safe.directory' is ignored in the repository
config: it can be under control of someone else, and thus a malicious
repositoy could trivially safe-list itself.  However, it's unclear (to
me) why 'git -c safe.directory=...' is ignored: 8959555cee
(setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02) only states that it's ignored, but doesn't
justify why.  Now, let's suppose that there was a compelling reason to
do so, e.g. in some way that I don't readily see it can be spoofed and
thus could be abused by an attacker to safe-list a malicious
repository.  If that were indeed the case, then couldn't
'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused
just as easily?

Or to approach it from the opposite direction: if such a
GIT_SAFE_DIRECTORIES environment variable is safe, then why should we
ignore 'safe.directory' in the command line, or in the environment
for that matter?

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> repositoy could trivially safe-list itself.  However, it's unclear (to
> me) why 'git -c safe.directory=...' is ignored: 8959555cee
> (setup_git_directory(): add an owner check for the top-level
> directory, 2022-03-02) only states that it's ignored, but doesn't
> justify why.  Now, let's suppose that there was a compelling reason to

Correct.  I do not think it is a restriction with any sensible
justification, just that it happened to be implemented that way.

IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user
interface than fixing the "we ignore 'git -c safe.directory'?"  bug.

Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

[Taylor Blau]

On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:
>   - it is interpreted as a regexp by 'grep', so, because of the dot,
>     it matches the name of the test script and the path of the trash
>     directory as well.  Consequently, these tests could be fooled by
>     any error message that would happen to include the path of the
>     test repository.

Nice catch. I wouldn't have even thought about "could match the test
directory" as a potential way for this test to generate a false
positive. So I'm happy to see us tightening this down.

Thanks.

Taylor

Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

[Carlo Marcelo Arenas Belón]

Apologies for the late feedback, hoping it is still timely though.

On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:
> 
>   - it only appears in the advice part, not in the actual error
>     message.
> 
>   - it is interpreted as a regexp by 'grep', so, because of the dot,
>     it matches the name of the test script and the path of the trash
>     directory as well.  Consequently, these tests could be fooled by
>     any error message that would happen to include the path of the
>     test repository.

The subject says something else and I think it is confusing because the
change you propose only seems to be removing the path as an implementation
detail while the core of the change seems to be making sure that the text
that is being matched is a "Fixed String" and part of the real error.

I agree is an improvement, but I think it could be done better by :

* using `grep -F` instead to make sure we don't interpret that string
  as a regex by mistake.
* including the path, so we are sure we really matched the error message
  that was really expected.  As a side effect we also validate this way
  that the code that does the lookup and reports the "name" of the
  repository that could be added to the exception list, reports the righ
  directory, which seems by itself an important thing we should want to
  avoid regressing against in future changes.

Both changed are implemented by the patch below and I think might be
worth considering as a replacement of this one.

Carlo

PS. Commit message could be improved, but wasn't sure how in the context
    of your series, so left it as a barebones one hoping it would be part
    of the patch you could adopt into your series.

----- >8 -----
Subject: t0033: verify detected worktree is accurate and part of the error
    
Change the helper function to check for a fixed message and make
sure to include the directory that we are testing from (which in
all current tests is the current directory).
    
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 239d93f4d2..c524b74b01 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER
 
 expect_rejected_dir () {
 	test_must_fail git status 2>err &&
-	grep "safe.directory" err
+	grep -F "unsafe repository ('$(pwd)'" err
 }
 
 test_expect_success 'safe.directory is not set' '

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[SZEDER Gábor]

On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
> > And we do need a subshell to set the
> > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> > config' would refuse to touch the config file.
> 
> Ah yes, of course.
>  
> > I think something like
> > 
> >   test_when_finished "(
> >         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
> >         git config --unset safe.directory
> >         )"
> > 
> > would work, though.
> 
> Would it be simpler to use this?
> 
> 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory

Oh, wow.  This is so obvious, no wonder it didn't occur to me :)

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

[SZEDER Gábor]

On Mon, May 09, 2022 at 02:45:21PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > repositoy could trivially safe-list itself.  However, it's unclear (to
> > me) why 'git -c safe.directory=...' is ignored: 8959555cee
> > (setup_git_directory(): add an owner check for the top-level
> > directory, 2022-03-02) only states that it's ignored, but doesn't
> > justify why.  Now, let's suppose that there was a compelling reason to
> 
> Correct.  I do not think it is a restriction with any sensible
> justification, just that it happened to be implemented that way.
> 
> IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user
> interface than fixing the "we ignore 'git -c safe.directory'?"  bug.

In light of this, I'm not sure we want to test said buggy behavior.
IOW, do we want 2/3 and 3/3 of this patch series at all?  Or perhaps
add those test as 'test_expect_failure' instead?

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Taylor Blau]

On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:
> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
> > > And we do need a subshell to set the
> > > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> > > config' would refuse to touch the config file.
> >
> > Ah yes, of course.
> >
> > > I think something like
> > >
> > >   test_when_finished "(
> > >         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
> > >         git config --unset safe.directory
> > >         )"
> > >
> > > would work, though.
> >
> > Would it be simpler to use this?
> >
> > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
>
> Oh, wow.  This is so obvious, no wonder it didn't occur to me :)

Don't we consider this one-shot environment variable to be sticky on
some shells (i.e., that it persists beyond just the "git config"
invocation here)?

Thanks,
Taylor

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Carlo Marcelo Arenas Belón]

On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:
> > >
> > > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
> >
> > Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> 
> Don't we consider this one-shot environment variable to be sticky on
> some shells (i.e., that it persists beyond just the "git config"
> invocation here)?

do you have an example of such a shell?, I would assume that since the
mechanism to implement these would be similar to local and we already
require local for running our tests, that shouldn't be an issue (at
least in the test suite), right?

any such variables should be only set as part of the environment used
by the posix shell before it call execve to invoke the next command IMHO.

Carlo

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Taylor Blau]

On Tue, May 10, 2022 at 01:06:58PM -0700, Carlo Marcelo Arenas Belón wrote:
> On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:
> > > >
> > > > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
> > >
> > > Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> >
> > Don't we consider this one-shot environment variable to be sticky on
> > some shells (i.e., that it persists beyond just the "git config"
> > invocation here)?
>
> do you have an example of such a shell?, I would assume that since the
> mechanism to implement these would be similar to local and we already
> require local for running our tests, that shouldn't be an issue (at
> least in the test suite), right?
>
> any such variables should be only set as part of the environment used
> by the posix shell before it call execve to invoke the next command IMHO.

This is completely my mistake, that stickiness exists only when invoking
shell _functions_, not other commands (like "git").

I have gotten so used to looking for the former, that I didn't read
carefully enough to realize that we are in the latter situation instead.

> Carlo

Thanks,
Taylor

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

[Eric Sunshine]

On 5/10/22 3:55 PM, Taylor Blau wrote:
> On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:
>> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
>>> Would it be simpler to use this?
>>>
>>> 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
>>
>> Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> 
> Don't we consider this one-shot environment variable to be sticky on
> some shells (i.e., that it persists beyond just the "git config"
> invocation here)?

Almost. Some old/buggy shells incorrectly allow the one-shot environment 
variable assignment to bleed past command invocation when the "command" 
is a shell function, like this:

     SOME_ENV_VAR=somevalue shell_function_call

We do "lint"[*] for that sort of problem in t/check-non-portable-shell.pl.

Stolee's example, however, is invoking a normal program (`git`, in this 
case), so isn't subject to that unwanted behavior from buggy shells.


[*]: a0a630192d (t/check-non-portable-shell: detect "FOO=bar 
shell_func", 2018-07-13)