Updates to the safe.directory config option
cover Updates to the safe.directory config option
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
Signature · 1 line
gitgitgadget
PATCH 1/3 t0033: add tests for safe.directory
From: Derrick Stolee <[email protected]>
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.
--- setup.c | 3 ++- t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100755 t/t0033-safe-directory.sh
setup.c +2 -1
@@ -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);
t/t0033-safe-directory.sh new file +34 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] t0033: add tests for safe.directory
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
It is difficult to change the ownership on a directory in our test suite, so insert a new
GIT_TEST_ASSUME_DIFFERENT_OWNERenvironment 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.
setup.c static int ensure_valid_ownership(const char *path) · line 1124
Quoted 4 lines
setup.c +2
-1-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?
Quoted 14 lines
t/t0033-safe-directory.sh new file +8
-0@@ -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
t/t0033-safe-directory.sh line 18
Quoted 10 lines
t/t0033-safe-directory.sh new file +10
-0+ +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.
t/t0033-safe-directory.sh line 34
Quoted 6 lines
t/t0033-safe-directory.sh new file +6
-0+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 1/3] t0033: add tests for safe.directory
Quoted 11 lines · Junio C Hamano
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
It is difficult to change the ownership on a directory in our test suite, so insert a new
GIT_TEST_ASSUME_DIFFERENT_OWNERenvironment 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.
Quoted 7 lines
setup.c static int ensure_valid_ownership(const char *path) · line 1124
Quoted 4 lines
setup.c +2
-1-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.
Quoted 29 lines
Quoted 14 lines
t/t0033-safe-directory.sh new file +8
-0@@ -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_OWNERt/t0033-safe-directory.sh line 18
Quoted 10 lines
t/t0033-safe-directory.sh new file +10
-0+ +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?
Quoted 3 lines
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 1/3] t0033: add tests for safe.directory
In reply to Derrick Stolee via GitGitGadgetQuoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
It is difficult to change the ownership on a directory in our test suite, so insert a new
GIT_TEST_ASSUME_DIFFERENT_OWNERenvironment 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..
t/t0033-safe-directory.sh line 8
Quoted 6 lines
t/t0033-safe-directory.sh new file +6
-0+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.
Quoted 6 lines
t/t0033-safe-directory.sh new file +6
-0+expect_rejected_dir () { + test_must_fail git status 2>err && + grep "safe.directory" err +} + +test_expect_success 'safe.directory is not set' '
t/t0033-safe-directory.sh line 25
Quoted 10 lines
t/t0033-safe-directory.sh new file +10
-0+ 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
Ævar Arnfjörð Bjarmason <[email protected]> writes:
(just this part)
Quoted 2 lines
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
In reply to Ævar Arnfjörð BjarmasonQuoted 16 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
It is difficult to change the ownership on a directory in our test suite, so insert a new
GIT_TEST_ASSUME_DIFFERENT_OWNERenvironment 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 oft/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..
Quoted 9 lines
t/t0033-safe-directory.sh line 8
Quoted 6 lines
t/t0033-safe-directory.sh new file +6
-0+test_description='verify safe.directory checks' + +. ./test-lib.sh + +GIT_TEST_ASSUME_DIFFERENT_OWNER=1 +export GIT_TEST_ASSUME_DIFFERENT_OWNERInstead of this "export" perhaps just add it in front of the "git status"?
If the only runs were in this helper below, then yes.
Quoted 4 lines
t/t0033-safe-directory.sh line 13
Quoted 4 lines
t/t0033-safe-directory.sh new file +4
-0+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.
Quoted 4 lines
t/t0033-safe-directory.sh line 25
Quoted 2 lines
t/t0033-safe-directory.sh new file +2
-0+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
PATCH 2/3 setup: fix safe.directory key not being checked
In reply to Derrick Stolee via GitGitGadgetFrom: Matheus Valadares <[email protected]>
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.
--- setup.c | 3 +++ t/t0033-safe-directory.sh | 5 +++++ 2 files changed, 8 insertions(+)
setup.c +3 -0
@@ -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 {
t/t0033-safe-directory.sh +5 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] setup: fix safe.directory key not being checked
"Matheus Valadares via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Matheus Valadares <[email protected]>
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.
Quoted 37 lines
Signed-off-by Matheus Valadares <[email protected]>Signed-off-by Derrick Stolee <[email protected]>--- setup.c | 3 +++ t/t0033-safe-directory.sh | 5 +++++ 2 files changed, 8 insertions(+)setup.c +3
-0@@ -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 {t/t0033-safe-directory.sh +5
-0@@ -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
PATCH 3/3 setup: opt-out of check with safe.directory=*
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- Documentation/config/safe.txt | 7 +++++++ setup.c | 6 ++++-- t/t0033-safe-directory.sh | 10 ++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-)
Documentation/config/safe.txt +7 -0
@@ -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.
setup.c +4 -2
@@ -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) &&
t/t0033-safe-directory.sh +10 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] setup: opt-out of check with safe.directory=*
Quoted 4 lines · Derrick Stolee via GitGitGadget
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.
Quoted 3 lines
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.
t/t0033-safe-directory.sh
Quoted 14 lines
setup.c +4
-2if (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) &&t/t0033-safe-directory.sh +0
-0
OK.
Quoted 6 lines
index
6f33c0dfefa..239d93f4d21100755t/t0033-safe-directory.sh +0
-0@@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' ' expect_rejected_dir '
t/t0033-safe-directory.sh test_expect_success 'safe.directory matches, but is reset' ' · line 47
Quoted 10 lines
t/t0033-safe-directory.sh +9
-0+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.
Quoted 1 line · Derrick Stolee via GitGitGadget
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"?
Quoted 11 lines
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.
Quoted 24 lines
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/14336Derrick 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 checkedDocumentation/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.shbase-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
Quoted 9 lines · Junio C Hamano
Quoted 1 line · Derrick Stolee via GitGitGadget
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.
Quoted 12 lines
Quoted 11 lines
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].
Quoted 8 lines
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
Quoted 3 lines · Derrick Stolee
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.
Quoted 5 lines
The tests that are added are in a new test file, so hopefully those don't collide with anything.
The changes in
setup.capply within theensure_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.
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee
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
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee
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.
Quoted 7 lines
Quoted 1 line
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
Quoted 15 lines · Taylor Blau
Quoted 14 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee
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.
Quoted 8 lines
Quoted 1 line
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.
cover t0033-safe-directory: test improvements and a documentation clarification
In reply to Derrick Stolee via GitGitGadgetAdd 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(-)
Signature · 1 line
2.36.0.676.gf39b21ed98
PATCH 1/3 t0033-safe-directory: check the error message without matching the trash dir
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.
--- t/t0033-safe-directory.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t0033-safe-directory.sh +1 -1
@@ -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' '
Signature · 1 line
2.36.0.676.gf39b21ed98
Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir
Quoted 5 lines · SZEDER Gábor
- 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
In reply to SZEDER GáborApologies for the late feedback, hoping it is still timely though.
Quoted 9 lines · SZEDER Gábor
- 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).
t/t0033-safe-directory.sh +1 -1
@@ -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' '
PATCH 2/3 t0033-safe-directory: check when 'safe.directory' is ignored
In reply to SZEDER GáborAccording 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.
--- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)
t/t0033-safe-directory.sh +13 -0
@@ -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
Signature · 1 line
2.36.0.676.gf39b21ed98
Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
Quoted 23 lines · 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 <[email protected]>--- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)t/t0033-safe-directory.sh +2
-0@@ -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 &&
t/t0033-safe-directory.sh test_expect_success 'safe.directory is not set' ' · line 30
Quoted 10 lines
t/t0033-safe-directory.sh +10
-0+ 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.
Quoted 3 lines
t/t0033-safe-directory.sh +0
-0test_expect_success 'safe.directory does not match' ' git config --global safe.directory bogus && expect_rejected_dir
Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
Quoted 37 lines · Junio C Hamano
Quoted 23 lines · 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 <[email protected]>--- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)t/t0033-safe-directory.sh +2
-0@@ -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 &&t/t0033-safe-directory.sh test_expect_success 'safe.directory is not set' ' · line 30
Quoted 10 lines
t/t0033-safe-directory.sh +10
-0+ 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
Quoted 8 lines
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 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
Quoted 47 lines · Derrick Stolee
Quoted 37 lines · Junio C Hamano
Quoted 23 lines · 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 <[email protected]>--- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)t/t0033-safe-directory.sh +2
-0@@ -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 &&t/t0033-safe-directory.sh test_expect_success 'safe.directory is not set' ' · line 30
Quoted 10 lines
t/t0033-safe-directory.sh +10
-0+ 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
Quoted 8 lines
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.
Quoted 2 lines
test_configwould do the same, right? I think it automatically does thetest_when_finishedfor 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
Quoted 49 lines · Junio C Hamano
Quoted 47 lines · Derrick Stolee
Quoted 45 lines · Junio C Hamano
Quoted 23 lines · 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 <[email protected]>--- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)t/t0033-safe-directory.sh +2
-0@@ -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 &&t/t0033-safe-directory.sh test_expect_success 'safe.directory is not set' ' · line 30
Quoted 10 lines
t/t0033-safe-directory.sh +10
-0+ 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.
Quoted 5 lines
Quoted 2 lines
test_configwould do the same, right? I think it automatically does thetest_when_finishedfor 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
Quoted 8 lines · SZEDER Gábor
Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Derrick Stolee
test_configwould do the same, right? I think it automatically does thetest_when_finishedfor us.I thought it (specifically, anything depends on
test_when_finished) has trouble working well from inside a subprocess?
Quoted 7 lines
Yeah,
test_configdoesn'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 in0968f12a99(test-lib-functions: detecttest_when_finishedin subshell, 2015-09-05) will kick in. And we do need a subshell to set the config, because without unsettingGIT_TEST_ASSUME_DIFFERENT_OWNER'git config' would refuse to touch the config file.
Ah yes, of course.
Quoted 8 lines
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 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
Quoted 13 lines · Derrick Stolee
Quoted 3 lines
And we do need a subshell to set the
config, because without unsettingGIT_TEST_ASSUME_DIFFERENT_OWNER'git config' would refuse to touch the config file.Ah yes, of course.
Quoted 7 lines
I think something like
test_when_finished "( unset GIT_TEST_ASSUME_DIFFERENT_USER && git config --unset safe.directory )"
Quoted 5 lines
Quoted 1 line
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 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
Quoted 14 lines · SZEDER Gábor
Quoted 13 lines · Derrick Stolee
Quoted 3 lines
And we do need a subshell to set the
config, because without unsettingGIT_TEST_ASSUME_DIFFERENT_OWNER'git config' would refuse to touch the config file.Ah yes, of course.
Quoted 7 lines
I think something like
test_when_finished "( unset GIT_TEST_ASSUME_DIFFERENT_USER && git config --unset safe.directory )"
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
would work, though.
Would it be simpler to use this?
GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directoryOh, 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
Quoted 8 lines · Taylor Blau
Quoted 4 lines
Quoted 2 lines
GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directoryOh, 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
Quoted 10 lines · Carlo Marcelo Arenas Belón
Quoted 8 lines · Taylor Blau
Quoted 4 lines
Quoted 2 lines
GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directoryOh, 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)?
Quoted 7 lines
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.
Quoted 1 line
Carlo
Thanks,
Taylor
Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored
In reply to Taylor BlauQuoted 11 lines · Taylor Blau
Quoted 6 lines · SZEDER Gábor
Quoted 3 lines · Derrick Stolee
Would it be simpler to use this?
GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directoryOh, 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)
PATCH 3/3 safe.directory: document and check that it's ignored in the environment
In reply to SZEDER GáborThe 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)
--- Documentation/config/safe.txt | 4 ++-- t/t0033-safe-directory.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)
Documentation/config/safe.txt +2 -2
@@ -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
t/t0033-safe-directory.sh +15 -0
@@ -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 &&
Signature · 1 line
2.36.0.676.gf39b21ed98
Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment
Quoted 5 lines · 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]).
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.
Quoted 26 lines
t/t0033-safe-directory.sh +15
-0@@ -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
Quoted 8 lines · Junio C Hamano
If we had
GIT_SAFE_DIRECTORIESthat 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 ...
Documentation/config/safe.txt override any such directories specified in the system config), add a · line 17
Quoted 12 lines
Documentation/config/safe.txt +2
-2@@ -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 3/3] safe.directory: document and check that it's ignored in the environment
Quoted 30 lines · Junio C Hamano
Quoted 8 lines · Junio C Hamano
If we had
GIT_SAFE_DIRECTORIESthat 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_COUNTand stuff are usable by end user scripts, but then ...Documentation/config/safe.txt override any such directories specified in the system config), add a · line 17
Quoted 12 lines
Documentation/config/safe.txt +2
-2@@ -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.
Quoted 6 lines
... 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_DIRECTORIESenvironment variable that should NOT be ignored.
I agree on both points.
Thanks for working on this!
-Stolee
Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment
In reply to Junio C HamanoQuoted 30 lines · Junio C Hamano
Quoted 8 lines · Junio C Hamano
If we had
GIT_SAFE_DIRECTORIESthat 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_COUNTand stuff are usable by end user scripts, but then ...Documentation/config/safe.txt override any such directories specified in the system config), add a · line 17
Quoted 12 lines
Documentation/config/safe.txt +2
-2@@ -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.
Quoted 2 lines
... 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".
Quoted 3 lines
or something, perhaps. I actually do think it is a useful addition to have
GIT_SAFE_DIRECTORIESenvironment 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
Quoted 5 lines · SZEDER Gábor
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 3/3] safe.directory: document and check that it's ignored in the environment
Quoted 8 lines · Junio C Hamano
Quoted 5 lines · SZEDER Gábor
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
Quoted 5 lines
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_DIRECTORIESmay 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?