{"thread":{"id":"57724","subject":"[PATCH 0/3] Updates to the safe.directory config option","startedAt":"2022-04-13T15:32:39Z","lastAt":"2022-05-10T20:18:41Z","messageCount":39,"participants":["Derrick Stolee via GitGitGadget","Matheus Valadares via GitGitGadget","Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason","Taylor Blau","SZEDER Gábor","Carlo Marcelo Arenas Belón","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"453486","messageId":"pull.1215.git.1649863951.gitgitgadget@gmail.com","threadId":"57724","inReplyTo":null,"subject":"[PATCH 0/3] Updates to the safe.directory config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-13T15:32:28Z","receivedAt":"2022-04-13T15:32:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is a very fast response to the security release yesterday.\n\nThe second patch here is an adaptation from a contributor who created a pull\nrequest against git/git [1]. I augmented the patch with a test (the test\ninfrastructure is added in patch 1).\n\nThe third patch is a change to the safe.directory config option to include a\npossible \"*\" value to completely opt-out of the check. This will be\nparticularly helpful for cases where users run Git commands within a\ncontainer. This container workflow always runs as a different user than the\nhost, but also the container does not have access to the host's system or\nglobal config files. It's also helpful for users who don't want to set the\nconfig for a large number of shared repositories [2].\n\nThanks, -Stolee\n\n[1] https://github.com/git/git/pull/1235 [2]\nhttps://github.com/git-for-windows/git/issues/3787 [3]\nhttps://github.com/desktop/desktop/issues/14336\n\nDerrick Stolee (2):\n t0033: add tests for safe.directory\n setup: opt-out of check with safe.directory=*\n\nMatheus Valadares (1):\n setup: fix safe.directory key not being checked\n\n Documentation/config/safe.txt | 7 +++++\n setup.c | 12 ++++++---\n t/t0033-safe-directory.sh | 49 +++++++++++++++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 3 deletions(-)\n create mode 100755 t/t0033-safe-directory.sh\n\n\nbase-commit: 11cfe552610386954886543f5de87dcc49ad5735\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1215\n-- \ngitgitgadget\n"},{"id":"453487","messageId":"5b18bd1852d673ab5c62a67f873987d74294cd70.1649863951.git.gitgitgadget@gmail.com","threadId":"57724","inReplyTo":"pull.1215.git.1649863951.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t0033: add tests for safe.directory","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-13T15:32:29Z","receivedAt":"2022-04-13T15:32:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt is difficult to change the ownership on a directory in our test\nsuite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment\nvariable to trick Git into thinking we are in a differently-owned\ndirectory. This allows us to test that the config is parsed correctly.\n\nSigned-off-by: Derrick Stolee \n---\n setup.c | 3 ++-\n t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n create mode 100755 t/t0033-safe-directory.sh\n\ndiff --git a/setup.c b/setup.c\nindex c8f67bfed52..f54f449008a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1119,7 +1119,8 @@ static int ensure_valid_ownership(const char *path)\n {\n \tstruct safe_directory_data data = { .path = path };\n \n-\tif (is_path_owned_by_current_user(path))\n+\tif (is_path_owned_by_current_user(path) &&\n+\t !git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0))\n \t\treturn 1;\n \n \tread_very_early_config(safe_directory_cb, &data);\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nnew file mode 100755\nindex 00000000000..9380ff3d017\n--- /dev/null\n+++ b/t/t0033-safe-directory.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='verify safe.directory checks'\n+\n+. ./test-lib.sh\n+\n+GIT_TEST_ASSUME_DIFFERENT_OWNER=1\n+export GIT_TEST_ASSUME_DIFFERENT_OWNER\n+\n+expect_rejected_dir () {\n+\ttest_must_fail git status 2>err &&\n+\tgrep \"safe.directory\" err\n+}\n+\n+test_expect_success 'safe.directory is not set' '\n+\texpect_rejected_dir\n+'\n+\n+test_expect_success 'safe.directory does not match' '\n+\tgit config --global safe.directory bogus &&\n+\texpect_rejected_dir\n+'\n+\n+test_expect_success 'safe.directory matches' '\n+\tgit config --global --add safe.directory \"$(pwd)\" &&\n+\tgit status\n+'\n+\n+test_expect_success 'safe.directory matches, but is reset' '\n+\tgit config --global --add safe.directory \"\" &&\n+\texpect_rejected_dir\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"453488","messageId":"8a06688c121195f21472e3e49efd2b034b1537fc.1649863951.git.gitgitgadget@gmail.com","threadId":"57724","inReplyTo":"pull.1215.git.1649863951.gitgitgadget@gmail.com","subject":"[PATCH 2/3] setup: fix safe.directory key not being checked","fromName":"Matheus Valadares via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-13T15:32:30Z","receivedAt":"2022-04-13T15:32:44Z","isPatch":true,"sender":{"key":"name:Matheus Valadares","avatar":null},"body":"From: Matheus Valadares \n\nIt seems that nothing is ever checking to make sure the safe directories\nin the configs actually have the key safe.directory, so some unrelated\nconfig that has a value with a certain directory would also make it a\nsafe directory.\n\nSigned-off-by: Matheus Valadares \nSigned-off-by: Derrick Stolee \n---\n setup.c | 3 +++\n t/t0033-safe-directory.sh | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex f54f449008a..a995c359c32 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n {\n \tstruct safe_directory_data *data = d;\n \n+\tif (strcmp(key, \"safe.directory\"))\n+\t\treturn 0;\n+\n \tif (!value || !*value)\n \t\tdata->is_safe = 0;\n \telse {\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 9380ff3d017..6f33c0dfefa 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '\n \texpect_rejected_dir\n '\n \n+test_expect_success 'path exist as different key' '\n+\tgit config --global foo.bar \"$(pwd)\" &&\n+\texpect_rejected_dir\n+'\n+\n test_expect_success 'safe.directory matches' '\n \tgit config --global --add safe.directory \"$(pwd)\" &&\n \tgit status\n-- \ngitgitgadget\n\n"},{"id":"453489","messageId":"a5faf3a1779b51195313794fa0a48b7e2009c01b.1649863951.git.gitgitgadget@gmail.com","threadId":"57724","inReplyTo":"pull.1215.git.1649863951.gitgitgadget@gmail.com","subject":"[PATCH 3/3] setup: opt-out of check with safe.directory=*","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-13T15:32:31Z","receivedAt":"2022-04-13T15:32:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWith the addition of the safe.directory in 8959555ce\n(setup_git_directory(): add an owner check for the top-level directory,\n2022-03-02) released in v2.35.2, we are receiving feedback from a\nvariety of users about the feature.\n\nSome users have a very large list of shared repositories and find it\ncumbersome to add this config for every one of them.\n\nIn a more difficult case, certain workflows involve running Git commands\nwithin containers. The container boundary prevents any global or system\nconfig from communicating `safe.directory` values from the host into the\ncontainer. Further, the container almost always runs as a different user\nthan the owner of the directory in the host.\n\nTo simplify the reactions necessary for these users, extend the\ndefinition of the safe.directory config value to include a possible '*'\nvalue. This value implies that all directories are safe, providing a\nsingle setting to opt-out of this protection.\n\nNote that an empty assignment of safe.directory clears all previous\nvalues, and this is already the case with the \"if (!value || !*value)\"\ncondition.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/safe.txt | 7 +++++++\n setup.c | 6 ++++--\n t/t0033-safe-directory.sh | 10 ++++++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\nindex 63597b2df8f..6d764fe0ccf 100644\n--- a/Documentation/config/safe.txt\n+++ b/Documentation/config/safe.txt\n@@ -19,3 +19,10 @@ line option `-c safe.directory=`.\n The value of this setting is interpolated, i.e. `~/` expands to a\n path relative to the home directory and `%(prefix)/` expands to a\n path relative to Git's (runtime) prefix.\n++\n+To completely opt-out of this security check, set `safe.directory` to the\n+string `*`. This will allow all repositories to be treated as if their\n+directory was listed in the `safe.directory` list. If `safe.directory=*`\n+is set in system config and you want to re-enable this protection, then\n+initialize your list with an empty value before listing the repositories\n+that you deem safe.\ndiff --git a/setup.c b/setup.c\nindex a995c359c32..a42b21307f7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1103,9 +1103,11 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n \tif (strcmp(key, \"safe.directory\"))\n \t\treturn 0;\n \n-\tif (!value || !*value)\n+\tif (!value || !*value) {\n \t\tdata->is_safe = 0;\n-\telse {\n+\t} else if (!strcmp(value, \"*\")) {\n+\t\tdata->is_safe = 1;\n+\t} else {\n \t\tconst char *interpolated = NULL;\n \n \t\tif (!git_config_pathname(&interpolated, key, value) &&\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 6f33c0dfefa..239d93f4d21 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '\n \texpect_rejected_dir\n '\n \n+test_expect_success 'safe.directory=*' '\n+\tgit config --global --add safe.directory \"*\" &&\n+\tgit status\n+'\n+\n+test_expect_success 'safe.directory=*, but is reset' '\n+\tgit config --global --add safe.directory \"\" &&\n+\texpect_rejected_dir\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"453494","messageId":"xmqq35iharig.fsf@gitster.g","threadId":"57724","inReplyTo":"pull.1215.git.1649863951.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T16:15:51Z","receivedAt":"2022-04-13T16:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Here is a very fast response to the security release yesterday.\n\nWow. While I were down the whole day yesterday after sending the\nrelease announcement, it seems a lot have happened X-<. Does your\n\"a very fast\" expect only \"wow, thanks for a fast reponse\", or does\nit also expect \"ok, we'll take a deep look with a spoonful of salt\nas it was prepared in haste\"?\n\n> The second patch here is an adaptation from a contributor who created a pull\n> request against git/git [1]. I augmented the patch with a test (the test\n> infrastructure is added in patch 1).\n>\n> The third patch is a change to the safe.directory config option to include a\n> possible \"*\" value to completely opt-out of the check. This will be\n> particularly helpful for cases where users run Git commands within a\n> container. This container workflow always runs as a different user than the\n> host, but also the container does not have access to the host's system or\n> global config files. It's also helpful for users who don't want to set the\n> config for a large number of shared repositories [2].\n\nLet me take a look how well these integrate into the maintenance\ntracks.\n\nI would appreciate something that is targetted and narrow that can\nbe applied to the oldest maintenance track (2.30.3) and then merged\nupwards, plus niceties on top that does not necessarily have to\napply to the oldest ones if the surrounding code or tests were\nchanged more recently.\n\nThanks.\n\n> Thanks, -Stolee\n>\n> [1] https://github.com/git/git/pull/1235 [2]\n> https://github.com/git-for-windows/git/issues/3787 [3]\n> https://github.com/desktop/desktop/issues/14336\n>\n> Derrick Stolee (2):\n> t0033: add tests for safe.directory\n> setup: opt-out of check with safe.directory=*\n>\n> Matheus Valadares (1):\n> setup: fix safe.directory key not being checked\n>\n> Documentation/config/safe.txt | 7 +++++\n> setup.c | 12 ++++++---\n> t/t0033-safe-directory.sh | 49 +++++++++++++++++++++++++++++++++++\n> 3 files changed, 65 insertions(+), 3 deletions(-)\n> create mode 100755 t/t0033-safe-directory.sh\n>\n>\n> base-commit: 11cfe552610386954886543f5de87dcc49ad5735\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1215\n"},{"id":"453496","messageId":"xmqqwnft9cjg.fsf@gitster.g","threadId":"57724","inReplyTo":"5b18bd1852d673ab5c62a67f873987d74294cd70.1649863951.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t0033: add tests for safe.directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T16:24:35Z","receivedAt":"2022-04-13T16:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> It is difficult to change the ownership on a directory in our test\n> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment\n> variable to trick Git into thinking we are in a differently-owned\n> directory. This allows us to test that the config is parsed correctly.\n\nOK.\n\n> -\tif (is_path_owned_by_current_user(path))\n> +\tif (is_path_owned_by_current_user(path) &&\n> +\t !git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0))\n> \t\treturn 1;\n\nShouldn't the overriding \"GIT_TEST_BLAH\" be checked before the\nreal logic kicks in, I wonder?\n\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> new file mode 100755\n> index 00000000000..9380ff3d017\n> --- /dev/null\n> +++ b/t/t0033-safe-directory.sh\n> @@ -0,0 +1,34 @@\n> +#!/bin/sh\n> +\n> +test_description='verify safe.directory checks'\n> +\n> +. ./test-lib.sh\n> +\n> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1\n> +export GIT_TEST_ASSUME_DIFFERENT_OWNER\n> +\n> +expect_rejected_dir () {\n> +\ttest_must_fail git status 2>err &&\n> +\tgrep \"safe.directory\" err\n> +}\n> +...\n> +test_expect_success 'safe.directory matches' '\n> +\tgit config --global --add safe.directory \"$(pwd)\" &&\n> +\tgit status\n> +'\n\nJust double checking, as I know you are much closer to the affected\nplatform than I'd ever be ;-) but is the use of $(pwd) safe and\ncorrect here?\n\nI always get confused between $(pwd) and $PWD, which does not make\nany difference on platforms I have access to, but makes difference\nto hurt Windows users.\n\n> +test_expect_success 'safe.directory matches, but is reset' '\n> +\tgit config --global --add safe.directory \"\" &&\n> +\texpect_rejected_dir\n> +'\n> +\n> +test_done\n\nThanks. This step should apply to maint-2.30 cleanly, I would\nthink.\n"},{"id":"453497","messageId":"e8383303-e82c-dc23-9f28-6ff566021e82@github.com","threadId":"57724","inReplyTo":"xmqq35iharig.fsf@gitster.g","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-13T16:25:40Z","receivedAt":"2022-04-13T16:25:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2022 12:15 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Here is a very fast response to the security release yesterday.\n> \n> Wow. While I were down the whole day yesterday after sending the\n> release announcement, it seems a lot have happened X-<. Does your\n> \"a very fast\" expect only \"wow, thanks for a fast reponse\", or does\n> it also expect \"ok, we'll take a deep look with a spoonful of salt\n> as it was prepared in haste\"?\n\nI tried to do my due diligence here, but I will admit to some amount\nof haste being applied due to the many distinct sources that have\nmotivated the change.\n\n>> The second patch here is an adaptation from a contributor who created a pull\n>> request against git/git [1]. I augmented the patch with a test (the test\n>> infrastructure is added in patch 1).\n>>\n>> The third patch is a change to the safe.directory config option to include a\n>> possible \"*\" value to completely opt-out of the check. This will be\n>> particularly helpful for cases where users run Git commands within a\n>> container. This container workflow always runs as a different user than the\n>> host, but also the container does not have access to the host's system or\n>> global config files. It's also helpful for users who don't want to set the\n>> config for a large number of shared repositories [2].\n> \n> Let me take a look how well these integrate into the maintenance\n> tracks.\n> \n> I would appreciate something that is targetted and narrow that can\n> be applied to the oldest maintenance track (2.30.3) and then merged\n> upwards, plus niceties on top that does not necessarily have to\n> apply to the oldest ones if the surrounding code or tests were\n> changed more recently.\n\nThe tests that are added are in a new test file, so hopefully\nthose don't collide with anything.\n\nThe changes in setup.c apply within the ensure_valid_ownership()\nso should apply to any versions that have the fix.\n\nThanks,\n-Stolee\n"},{"id":"453498","messageId":"7f5bebcd-5622-51c7-2904-b793337843b1@github.com","threadId":"57724","inReplyTo":"xmqqwnft9cjg.fsf@gitster.g","subject":"Re: [PATCH 1/3] t0033: add tests for safe.directory","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-13T16:29:49Z","receivedAt":"2022-04-13T16:29:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2022 12:24 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> It is difficult to change the ownership on a directory in our test\n>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment\n>> variable to trick Git into thinking we are in a differently-owned\n>> directory. This allows us to test that the config is parsed correctly.\n> \n> OK.\n> \n>> -\tif (is_path_owned_by_current_user(path))\n>> +\tif (is_path_owned_by_current_user(path) &&\n>> +\t !git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0))\n>> \t\treturn 1;\n> \n> Shouldn't the overriding \"GIT_TEST_BLAH\" be checked before the\n> real logic kicks in, I wonder?\n\nEither order would work. I bet that checking the environment is\nfaster than checking the disk, so swapping the order would be prudent\nhere.\n \n>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n>> new file mode 100755\n>> index 00000000000..9380ff3d017\n>> --- /dev/null\n>> +++ b/t/t0033-safe-directory.sh\n>> @@ -0,0 +1,34 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='verify safe.directory checks'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1\n>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER\n>> +\n>> +expect_rejected_dir () {\n>> +\ttest_must_fail git status 2>err &&\n>> +\tgrep \"safe.directory\" err\n>> +}\n>> +...\n>> +test_expect_success 'safe.directory matches' '\n>> +\tgit config --global --add safe.directory \"$(pwd)\" &&\n>> +\tgit status\n>> +'\n> \n> Just double checking, as I know you are much closer to the affected\n> platform than I'd ever be ;-) but is the use of $(pwd) safe and\n> correct here?\n> \n> I always get confused between $(pwd) and $PWD, which does not make\n> any difference on platforms I have access to, but makes difference\n> to hurt Windows users.\n\nThese tests pass CI on Windows. I've had issues before using $PWD,\nthinking back to tests in t7900-maintenance.sh that use $(pwd)\ninstead.\n\nThanks,\n-Stolee\n"},{"id":"453499","messageId":"xmqqr1619c2v.fsf@gitster.g","threadId":"57724","inReplyTo":"8a06688c121195f21472e3e49efd2b034b1537fc.1649863951.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] setup: fix safe.directory key not being checked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T16:34:32Z","receivedAt":"2022-04-13T16:34:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matheus Valadares via GitGitGadget\" \nwrites:\n\n> From: Matheus Valadares \n>\n> It seems that nothing is ever checking to make sure the safe directories\n> in the configs actually have the key safe.directory, so some unrelated\n> config that has a value with a certain directory would also make it a\n> safe directory.\n\nGood finding, and the fix is straight-forward and obviously correct.\nThanks.\n\n> Signed-off-by: Matheus Valadares \n> Signed-off-by: Derrick Stolee \n> ---\n> setup.c | 3 +++\n> t/t0033-safe-directory.sh | 5 +++++\n> 2 files changed, 8 insertions(+)\n\n> diff --git a/setup.c b/setup.c\n> index f54f449008a..a995c359c32 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n> {\n> \tstruct safe_directory_data *data = d;\n> \n> +\tif (strcmp(key, \"safe.directory\"))\n> +\t\treturn 0;\n> +\n> \tif (!value || !*value)\n> \t\tdata->is_safe = 0;\n> \telse {\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index 9380ff3d017..6f33c0dfefa 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '\n> \texpect_rejected_dir\n> '\n> \n> +test_expect_success 'path exist as different key' '\n> +\tgit config --global foo.bar \"$(pwd)\" &&\n> +\texpect_rejected_dir\n> +'\n> +\n> test_expect_success 'safe.directory matches' '\n> \tgit config --global --add safe.directory \"$(pwd)\" &&\n> \tgit status\n"},{"id":"453501","messageId":"xmqqk0bt9bsb.fsf@gitster.g","threadId":"57724","inReplyTo":"a5faf3a1779b51195313794fa0a48b7e2009c01b.1649863951.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] setup: opt-out of check with safe.directory=*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T16:40:52Z","receivedAt":"2022-04-13T16:41:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> To simplify the reactions necessary for these users, extend the\n> definition of the safe.directory config value to include a possible '*'\n> value. This value implies that all directories are safe, providing a\n> single setting to opt-out of this protection.\n\nOK. During the development of the original fix, we discussed if a\nmore flexible mechanism, like allowing globs, but ended up with the\nsimplest and easiest to explain option, with the expectation that we\nmay want to loosen it later as necessary. And this is certainly\nwhat we would have expected to add.\n\n> Note that an empty assignment of safe.directory clears all previous\n> values, and this is already the case with the \"if (!value || !*value)\"\n> condition.\n\nOK.\n\n> \tif (strcmp(key, \"safe.directory\"))\n> \t\treturn 0;\n> \n> -\tif (!value || !*value)\n> +\tif (!value || !*value) {\n> \t\tdata->is_safe = 0;\n> -\telse {\n> +\t} else if (!strcmp(value, \"*\")) {\n> +\t\tdata->is_safe = 1;\n> +\t} else {\n> \t\tconst char *interpolated = NULL;\n> \n> \t\tif (!git_config_pathname(&interpolated, key, value) &&\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n\nOK.\n\n> index 6f33c0dfefa..239d93f4d21 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '\n> \texpect_rejected_dir\n> '\n> \n> +test_expect_success 'safe.directory=*' '\n> +\tgit config --global --add safe.directory \"*\" &&\n> +\tgit status\n> +'\n> +\n> +test_expect_success 'safe.directory=*, but is reset' '\n> +\tgit config --global --add safe.directory \"\" &&\n> +\texpect_rejected_dir\n> +'\n\nThanks.\n"},{"id":"453503","messageId":"xmqqee219bly.fsf@gitster.g","threadId":"57724","inReplyTo":"e8383303-e82c-dc23-9f28-6ff566021e82@github.com","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T16:44:41Z","receivedAt":"2022-04-13T16:45:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I tried to do my due diligence here, but I will admit to some amount\n> of haste being applied due to the many distinct sources that have\n> motivated the change.\n\nOK. Thanks for a fast response to a brown-paper-bag bug or two.\n\n> The tests that are added are in a new test file, so hopefully\n> those don't collide with anything.\n>\n> The changes in setup.c apply within the ensure_valid_ownership()\n> so should apply to any versions that have the fix.\n\nYup, I was more worried about newer test helpers and API functions\nthat did not exist in the older code base, but after reading the\npatches through, I think these are all from battle tested and solid\npart that applies to all relevant maintenance tracks.\n\nThanks.\n"},{"id":"453521","messageId":"220413.86k0bseqq8.gmgdl@evledraar.gmail.com","threadId":"57724","inReplyTo":"5b18bd1852d673ab5c62a67f873987d74294cd70.1649863951.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t0033: add tests for safe.directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-13T19:16:20Z","receivedAt":"2022-04-13T19:19:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> It is difficult to change the ownership on a directory in our test\n> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment\n> variable to trick Git into thinking we are in a differently-owned\n> directory. This allows us to test that the config is parsed correctly.\n\nI think this is a good trade-off, but FWIW I'd think we could test also\nwithout the git_env_bool() by having the test depend on !NOT_ROOT, then\ncheck the owner of t/test-lib.sh, and chown to that user (i.e. the\n\"real\" user).\n\nBut that's all sorts of more fragile than just this test variable..\n> +test_description='verify safe.directory checks'\n> +\n> +. ./test-lib.sh\n> +\n> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1\n> +export GIT_TEST_ASSUME_DIFFERENT_OWNER\n\nInstead of this \"export\" perhaps just add it in front of the \"git\nstatus\"?\n\nThese tests also pass with SANITIZE=leak, so please add\nTEST_PASSES_SANITIZE_LEAK=true at the top.\n\n> +expect_rejected_dir () {\n> +\ttest_must_fail git status 2>err &&\n> +\tgrep \"safe.directory\" err\n> +}\n> +\n> +test_expect_success 'safe.directory is not set' '\n> +\texpect_rejected_dir\n> +'\n> +\n> +test_expect_success 'safe.directory does not match' '\n> +\tgit config --global safe.directory bogus &&\n> +\texpect_rejected_dir\n> +'\n> +\n> +test_expect_success 'safe.directory matches' '\n> +\tgit config --global --add safe.directory \"$(pwd)\" &&\n\nnit: $PWD instead of $(pwd)\n"},{"id":"453524","messageId":"xmqqbkx49369.fsf@gitster.g","threadId":"57724","inReplyTo":"220413.86k0bseqq8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] t0033: add tests for safe.directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T19:46:54Z","receivedAt":"2022-04-13T19:47:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n(just this part)\n\n> These tests also pass with SANITIZE=leak, so please add\n> TEST_PASSES_SANITIZE_LEAK=true at the top.\n\nDerrick, please ignore the above. It is totally outside the scope\nof these patches, and they are meant to be applied on top of the\n2.30 maintenance track, where TEST_PASSES_SANITIZE_LEAK=true was\nirrelevant.\n\nI do not mind adding such after the dust settles on top of 'master'\n(or possibly 'maint'), but not as part of these \"let's fix the screw\nup in 2.30.3 and its friends\" effort.\n\nThanks.\n"},{"id":"453556","messageId":"6b778cfe-2549-854a-7b77-58f815412ca1@github.com","threadId":"57724","inReplyTo":"220413.86k0bseqq8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] t0033: add tests for safe.directory","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-13T19:52:29Z","receivedAt":"2022-04-13T19:54:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2022 3:16 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> It is difficult to change the ownership on a directory in our test\n>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment\n>> variable to trick Git into thinking we are in a differently-owned\n>> directory. This allows us to test that the config is parsed correctly.\n> \n> I think this is a good trade-off, but FWIW I'd think we could test also\n> without the git_env_bool() by having the test depend on !NOT_ROOT, then\n> check the owner of t/test-lib.sh, and chown to that user (i.e. the\n> \"real\" user).\n> \n> But that's all sorts of more fragile than just this test variable..\n>> +test_description='verify safe.directory checks'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1\n>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER\n> \n> Instead of this \"export\" perhaps just add it in front of the \"git\n> status\"?\n\nIf the only runs were in this helper below, then yes.\n\n>> +expect_rejected_dir () {\n>> +\ttest_must_fail git status 2>err &&\n>> +\tgrep \"safe.directory\" err\n>> +}\n\nLater patches add more success cases that run 'git status'\nas its verification that the match works. I didn't think it\nwas good to have this environment variable set for each of\nthose invocations.\n\nThis script has one purpose, and this environment variable\nis required to make any of the checks work. Setting it\nglobally seems the best way to do that.\n\n>> +test_expect_success 'safe.directory matches' '\n>> +\tgit config --global --add safe.directory \"$(pwd)\" &&\n> \n> nit: $PWD instead of $(pwd)\n\nHistorically, $PWD doesn't work properly across platforms,\nso I have used $(pwd) consistently across many contributions.\n\nThanks,\n-Stolee\n"},{"id":"453594","messageId":"xmqqmtgo7mqu.fsf@gitster.g","threadId":"57724","inReplyTo":"xmqqee219bly.fsf@gitster.g","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T20:27:05Z","receivedAt":"2022-04-13T20:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Derrick Stolee writes:\n>\n>> I tried to do my due diligence here, but I will admit to some amount\n>> of haste being applied due to the many distinct sources that have\n>> motivated the change.\n>\n> OK. Thanks for a fast response to a brown-paper-bag bug or two.\n\nHere is a draft release notes to 2.30.4.txt; I may tag this later\ntonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together\nwith the result of merging the fixes into newer maintenance releases.\n\nThanks, everybody, for working on the issue.\n\n\nGit v2.30.4 Release Notes (draft)\n=================================\n\nThis release contains minor fix-ups for the changes that went into\nGit 2.30.3, which was made to address CVE-2022-24765.\n\n * The code that was meant to parse the new `safe.directory`\n configuration variable was not checking what configuration\n variable was being fed to it, which has been corrected.\n\n * '*' can be used as the value for the `safe.directory` variable to\n signal that the user considers that any directory is safe.\n\n\nDerrick Stolee (2):\n t0033: add tests for safe.directory\n setup: opt-out of check with safe.directory=*\n\nMatheus Valadares (1):\n setup: fix safe.directory key not being checked\n"},{"id":"453602","messageId":"Ylc/4Spmtw/f5Nwr@nand.local","threadId":"57724","inReplyTo":"xmqqmtgo7mqu.fsf@gitster.g","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-13T21:25:53Z","receivedAt":"2022-04-13T21:25:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > Derrick Stolee writes:\n> >\n> >> I tried to do my due diligence here, but I will admit to some amount\n> >> of haste being applied due to the many distinct sources that have\n> >> motivated the change.\n> >\n> > OK. Thanks for a fast response to a brown-paper-bag bug or two.\n>\n> Here is a draft release notes to 2.30.4.txt; I may tag this later\n> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together\n> with the result of merging the fixes into newer maintenance releases.\n>\n> Thanks, everybody, for working on the issue.\n\nThanks; this all looks good to me. I took a careful look at Stolee's new\npatches as well as the (elided) release notes from you below, and both\nlook good to me.\n\nDon't worry about applying my Reviewed-by, but I did want to acknowledge\nhaving looked at these.\n\nThanks,\nTaylor\n"},{"id":"453605","messageId":"xmqqwnfs64ki.fsf@gitster.g","threadId":"57724","inReplyTo":"Ylc/4Spmtw/f5Nwr@nand.local","subject":"Re: [PATCH 0/3] Updates to the safe.directory config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T21:45:01Z","receivedAt":"2022-04-13T21:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>> > Derrick Stolee writes:\n>> >\n>> >> I tried to do my due diligence here, but I will admit to some amount\n>> >> of haste being applied due to the many distinct sources that have\n>> >> motivated the change.\n>> >\n>> > OK. Thanks for a fast response to a brown-paper-bag bug or two.\n>>\n>> Here is a draft release notes to 2.30.4.txt; I may tag this later\n>> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together\n>> with the result of merging the fixes into newer maintenance releases.\n>>\n>> Thanks, everybody, for working on the issue.\n>\n> Thanks; this all looks good to me. I took a careful look at Stolee's new\n> patches as well as the (elided) release notes from you below, and both\n> look good to me.\n>\n> Don't worry about applying my Reviewed-by, but I did want to acknowledge\n> having looked at these.\n\nThanks for giving an extra set of eyes.\n\n"},{"id":"454512","messageId":"20220427170649.4949-1-szeder.dev@gmail.com","threadId":"57724","inReplyTo":"pull.1215.git.1649863951.gitgitgadget@gmail.com","subject":"[PATCH 0/3] t0033-safe-directory: test improvements and a documentation clarification","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-04-27T17:06:46Z","receivedAt":"2022-04-27T17:07:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Add coverage for the cases when 'safe.directory' is ignored, a minor\ntest improvement, and a small documentation clarification.\n\nSZEDER Gábor (3):\n t0033-safe-directory: check the error message without matching the\n trash dir\n t0033-safe-directory: check when 'safe.directory' is ignored\n safe.directory: document and check that it's ignored in the\n environment\n\n Documentation/config/safe.txt | 4 ++--\n t/t0033-safe-directory.sh | 30 +++++++++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 3 deletions(-)\n\n-- \n2.36.0.676.gf39b21ed98\n\n"},{"id":"454513","messageId":"20220427170649.4949-2-szeder.dev@gmail.com","threadId":"57724","inReplyTo":"20220427170649.4949-1-szeder.dev@gmail.com","subject":"[PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-04-27T17:06:47Z","receivedAt":"2022-04-27T17:07:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Since 8959555cee (setup_git_directory(): add an owner check for the\ntop-level directory, 2022-03-02) when git finds itself in a repository\nowned by someone else, it aborts with a \"fatal: unsafe repository\n()\" error message and an advice about how to set the\n'safe.directory' config variable to mark that repository as safe.\n't0033-safe-directory.sh' contains tests that check that this feature\nand handling said config work as intended. To ensure that git dies\nfor the right reason, several of those tests check that its standard\nerror contains the name of that config variable, but:\n\n - it only appears in the advice part, not in the actual error\n message.\n\n - it is interpreted as a regexp by 'grep', so, because of the dot,\n it matches the name of the test script and the path of the trash\n directory as well. Consequently, these tests could be fooled by\n any error message that would happen to include the path of the\n test repository.\n\nTighten these checks to look for \"unsafe repository\" instead.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t0033-safe-directory.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 239d93f4d2..6f9680e8b0 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER\n \n expect_rejected_dir () {\n \ttest_must_fail git status 2>err &&\n-\tgrep \"safe.directory\" err\n+\tgrep \"unsafe repository\" err\n }\n \n test_expect_success 'safe.directory is not set' '\n-- \n2.36.0.676.gf39b21ed98\n\n"},{"id":"454514","messageId":"20220427170649.4949-3-szeder.dev@gmail.com","threadId":"57724","inReplyTo":"20220427170649.4949-1-szeder.dev@gmail.com","subject":"[PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-04-27T17:06:48Z","receivedAt":"2022-04-27T17:07:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"According to the documentation 'safe.directory' \"is only respected\nwhen specified in a system or global config, not when it is specified\nin a repository config or via the command line option -c\nsafe.directory=\".\n\nAdd tests to check that 'safe.directory' in the repository config or\non the command line is indeed ignored.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t0033-safe-directory.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 6f9680e8b0..82dac0eb93 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '\n \texpect_rejected_dir\n '\n \n+test_expect_success 'ignoring safe.directory on the command line' '\n+\ttest_must_fail git -c safe.directory=\"$(pwd)\" status 2>err &&\n+\tgrep \"unsafe repository\" err\n+'\n+\n+test_expect_success 'ignoring safe.directory in repo config' '\n+\t(\n+\t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config safe.directory \"$(pwd)\"\n+\t) &&\n+\texpect_rejected_dir\n+'\n+\n test_expect_success 'safe.directory does not match' '\n \tgit config --global safe.directory bogus &&\n \texpect_rejected_dir\n-- \n2.36.0.676.gf39b21ed98\n\n"},{"id":"454515","messageId":"20220427170649.4949-4-szeder.dev@gmail.com","threadId":"57724","inReplyTo":"20220427170649.4949-1-szeder.dev@gmail.com","subject":"[PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-04-27T17:06:49Z","receivedAt":"2022-04-27T17:07:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The description of 'safe.directory' mentions that it's respected in\nthe system and global configs, and ignored in the repository config\nand on the command line, but it doesn't mention whether it's respected\nor ignored when specified via environment variables (nor does the\ncommit message adding 'safe.directory' [1]).\n\nClarify that 'safe.directory' is ignored when specified in the\nenvironment, and add tests to make sure that it remains so.\n\n[1] 8959555cee (setup_git_directory(): add an owner check for the\n top-level directory, 2022-03-02)\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/config/safe.txt | 4 ++--\n t/t0033-safe-directory.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\nindex 6d764fe0cc..ae0e2e3bdb 100644\n--- a/Documentation/config/safe.txt\n+++ b/Documentation/config/safe.txt\n@@ -13,8 +13,8 @@ override any such directories specified in the system config), add a\n `safe.directory` entry with an empty value.\n +\n This config setting is only respected when specified in a system or global\n-config, not when it is specified in a repository config or via the command\n-line option `-c safe.directory=`.\n+config, not when it is specified in a repository config, via the command\n+line option `-c safe.directory=`, or in environment variables.\n +\n The value of this setting is interpolated, i.e. `~/` expands to a\n path relative to the home directory and `%(prefix)/` expands to a\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 82dac0eb93..238b25f91a 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '\n \tgrep \"unsafe repository\" err\n '\n \n+test_expect_success 'ignoring safe.directory in the environment' '\n+\ttest_must_fail env GIT_CONFIG_COUNT=1 \\\n+\t\tGIT_CONFIG_KEY_0=\"safe.directory\" \\\n+\t\tGIT_CONFIG_VALUE_0=\"$(pwd)\" \\\n+\t\tgit status 2>err &&\n+\tgrep \"unsafe repository\" err\n+'\n+\n+test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '\n+\ttest_must_fail env \\\n+\t\tGIT_CONFIG_PARAMETERS=\"${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}\" \\\n+\t\tgit status 2>err &&\n+\tgrep \"unsafe repository\" err\n+'\n+\n test_expect_success 'ignoring safe.directory in repo config' '\n \t(\n \t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n-- \n2.36.0.676.gf39b21ed98\n\n"},{"id":"454542","messageId":"xmqqr15il0uo.fsf@gitster.g","threadId":"57724","inReplyTo":"20220427170649.4949-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-27T20:37:03Z","receivedAt":"2022-04-27T20:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> According to the documentation 'safe.directory' \"is only respected\n> when specified in a system or global config, not when it is specified\n> in a repository config or via the command line option -c\n> safe.directory=\".\n>\n> Add tests to check that 'safe.directory' in the repository config or\n> on the command line is indeed ignored.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t0033-safe-directory.sh | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n>\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index 6f9680e8b0..82dac0eb93 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '\n> \texpect_rejected_dir\n> '\n> \n> +test_expect_success 'ignoring safe.directory on the command line' '\n> +\ttest_must_fail git -c safe.directory=\"$(pwd)\" status 2>err &&\n> +\tgrep \"unsafe repository\" err\n> +'\n> +\n> +test_expect_success 'ignoring safe.directory in repo config' '\n> +\t(\n> +\t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config safe.directory \"$(pwd)\"\n> +\t) &&\n> +\texpect_rejected_dir\n> +'\n\nI am debating myself if we want to remove the in-repository\nsafe.directory configuration setting after this test piece is done,\nwith test_when_finished. We just made sure, with this test, that\nhaving the variable does not affect anything, so the subsequent\ntests should not care hence it is probably OK. On the other hand,\nto make sure those settings they make (e.g. setting it globally is\nwhat the next test we can see below does) is what affects their\noutcome, it removes one more thing we need to worry about if we\nclean after ourselves. I dunno.\n\nOther than that, looking good so far; both [1/3] and [2/3] look good.\n\n> test_expect_success 'safe.directory does not match' '\n> \tgit config --global safe.directory bogus &&\n> \texpect_rejected_dir\n"},{"id":"454544","messageId":"xmqqlevql0lj.fsf@gitster.g","threadId":"57724","inReplyTo":"20220427170649.4949-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-27T20:42:32Z","receivedAt":"2022-04-27T20:42:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The description of 'safe.directory' mentions that it's respected in\n> the system and global configs, and ignored in the repository config\n> and on the command line, but it doesn't mention whether it's respected\n> or ignored when specified via environment variables (nor does the\n> commit message adding 'safe.directory' [1]).\n\nIf we had GIT_SAFE_DIRECTORIES that lists the safe directories (like\n$PATH does), that would have been absolutely necessary to document\nhow it works, but GIT_CONFIG_* is merely an implementation detail of\nhow \"git -c var=val\" works and I am not sure if it is even a good\nidea to hardcode how they happen to work like these tests. The only\nthing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are\nused internally by the implementation and they should not muck with\nit, no?\n\nI am moderately negative about this change.\n\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index 82dac0eb93..238b25f91a 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '\n> \tgrep \"unsafe repository\" err\n> '\n> \n> +test_expect_success 'ignoring safe.directory in the environment' '\n> +\ttest_must_fail env GIT_CONFIG_COUNT=1 \\\n> +\t\tGIT_CONFIG_KEY_0=\"safe.directory\" \\\n> +\t\tGIT_CONFIG_VALUE_0=\"$(pwd)\" \\\n> +\t\tgit status 2>err &&\n> +\tgrep \"unsafe repository\" err\n> +'\n> +\n> +test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '\n> +\ttest_must_fail env \\\n> +\t\tGIT_CONFIG_PARAMETERS=\"${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}\" \\\n> +\t\tgit status 2>err &&\n> +\tgrep \"unsafe repository\" err\n> +'\n> +\n> test_expect_success 'ignoring safe.directory in repo config' '\n> \t(\n> \t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n"},{"id":"454546","messageId":"xmqqee1il09v.fsf@gitster.g","threadId":"57724","inReplyTo":"xmqqlevql0lj.fsf@gitster.g","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-27T20:49:32Z","receivedAt":"2022-04-27T20:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like\n> $PATH does), that would have been absolutely necessary to document\n> how it works, but GIT_CONFIG_* is merely an implementation detail of\n> how \"git -c var=val\" works and I am not sure if it is even a good\n> idea to hardcode how they happen to work like these tests. The only\n> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are\n> used internally by the implementation and they should not muck with\n> it, no?\n\nI misremembered. GIT_CONFIG_COUNT and stuff are usable by end user\nscripts, but then ...\n\n> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\n> index 6d764fe0cc..ae0e2e3bdb 100644\n> --- a/Documentation/config/safe.txt\n> +++ b/Documentation/config/safe.txt\n> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a\n> `safe.directory` entry with an empty value.\n> +\n> This config setting is only respected when specified in a system or global\n> -config, not when it is specified in a repository config or via the command\n> -line option `-c safe.directory=`.\n> +config, not when it is specified in a repository config, via the command\n> +line option `-c safe.directory=`, or in environment variables.\n\n... this part must clarify what environment variables it is talking\nabout.\n\n ... via the command line option `-c safe.directory=`, or\n via the GIT_CONFIG_{KEY,VALUE} mechanism.\n\nor something, perhaps. I actually do think it is a useful addition\nto have GIT_SAFE_DIRECTORIES environment variable that should NOT be\nignored.\n"},{"id":"454620","messageId":"a44b4f0e-038c-9bcd-80d6-f53c87bd7f2d@github.com","threadId":"57724","inReplyTo":"xmqqr15il0uo.fsf@gitster.g","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-29T16:12:40Z","receivedAt":"2022-04-29T16:12:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/27/2022 4:37 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n>> According to the documentation 'safe.directory' \"is only respected\n>> when specified in a system or global config, not when it is specified\n>> in a repository config or via the command line option -c\n>> safe.directory=\".\n>>\n>> Add tests to check that 'safe.directory' in the repository config or\n>> on the command line is indeed ignored.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> t/t0033-safe-directory.sh | 13 +++++++++++++\n>> 1 file changed, 13 insertions(+)\n>>\n>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n>> index 6f9680e8b0..82dac0eb93 100755\n>> --- a/t/t0033-safe-directory.sh\n>> +++ b/t/t0033-safe-directory.sh\n>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '\n>> \texpect_rejected_dir\n>> '\n>> \n>> +test_expect_success 'ignoring safe.directory on the command line' '\n>> +\ttest_must_fail git -c safe.directory=\"$(pwd)\" status 2>err &&\n>> +\tgrep \"unsafe repository\" err\n>> +'\n>> +\n>> +test_expect_success 'ignoring safe.directory in repo config' '\n>> +\t(\n>> +\t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n>> +\t\tgit config safe.directory \"$(pwd)\"\n>> +\t) &&\n>> +\texpect_rejected_dir\n>> +'\n> \n> I am debating myself if we want to remove the in-repository\n> safe.directory configuration setting after this test piece is done,\n> with test_when_finished. We just made sure, with this test, that\n> having the variable does not affect anything, so the subsequent\n> tests should not care hence it is probably OK. On the other hand,\n> to make sure those settings they make (e.g. setting it globally is\n> what the next test we can see below does) is what affects their\n> outcome, it removes one more thing we need to worry about if we\n> clean after ourselves. I dunno.\n\ntest_config would do the same, right? I think it automatically\ndoes the test_when_finished for us.\n\nThanks,\n-Stolee\n"},{"id":"454621","messageId":"9b0cc160-2d7f-6a38-c2d2-66ed8619cd17@github.com","threadId":"57724","inReplyTo":"xmqqee1il09v.fsf@gitster.g","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-29T16:13:55Z","receivedAt":"2022-04-29T16:14:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/27/2022 4:49 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like\n>> $PATH does), that would have been absolutely necessary to document\n>> how it works, but GIT_CONFIG_* is merely an implementation detail of\n>> how \"git -c var=val\" works and I am not sure if it is even a good\n>> idea to hardcode how they happen to work like these tests. The only\n>> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are\n>> used internally by the implementation and they should not muck with\n>> it, no?\n> \n> I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user\n> scripts, but then ...\n> \n>> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\n>> index 6d764fe0cc..ae0e2e3bdb 100644\n>> --- a/Documentation/config/safe.txt\n>> +++ b/Documentation/config/safe.txt\n>> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a\n>> `safe.directory` entry with an empty value.\n>> +\n>> This config setting is only respected when specified in a system or global\n>> -config, not when it is specified in a repository config or via the command\n>> -line option `-c safe.directory=`.\n>> +config, not when it is specified in a repository config, via the command\n>> +line option `-c safe.directory=`, or in environment variables.\n> \n> ... this part must clarify what environment variables it is talking\n> about.\n> \n> ... via the command line option `-c safe.directory=`, or\n> via the GIT_CONFIG_{KEY,VALUE} mechanism.\n> \n> or something, perhaps. I actually do think it is a useful addition\n> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be\n> ignored.\n\nI agree on both points.\n\nThanks for working on this!\n-Stolee\n"},{"id":"454628","messageId":"xmqq8rrndb6h.fsf@gitster.g","threadId":"57724","inReplyTo":"a44b4f0e-038c-9bcd-80d6-f53c87bd7f2d@github.com","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-29T17:57:58Z","receivedAt":"2022-04-29T17:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 4/27/2022 4:37 PM, Junio C Hamano wrote:\n>> SZEDER Gábor writes:\n>> \n>>> According to the documentation 'safe.directory' \"is only respected\n>>> when specified in a system or global config, not when it is specified\n>>> in a repository config or via the command line option -c\n>>> safe.directory=\".\n>>>\n>>> Add tests to check that 'safe.directory' in the repository config or\n>>> on the command line is indeed ignored.\n>>>\n>>> Signed-off-by: SZEDER Gábor \n>>> ---\n>>> t/t0033-safe-directory.sh | 13 +++++++++++++\n>>> 1 file changed, 13 insertions(+)\n>>>\n>>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n>>> index 6f9680e8b0..82dac0eb93 100755\n>>> --- a/t/t0033-safe-directory.sh\n>>> +++ b/t/t0033-safe-directory.sh\n>>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '\n>>> \texpect_rejected_dir\n>>> '\n>>> \n>>> +test_expect_success 'ignoring safe.directory on the command line' '\n>>> +\ttest_must_fail git -c safe.directory=\"$(pwd)\" status 2>err &&\n>>> +\tgrep \"unsafe repository\" err\n>>> +'\n>>> +\n>>> +test_expect_success 'ignoring safe.directory in repo config' '\n>>> +\t(\n>>> +\t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n>>> +\t\tgit config safe.directory \"$(pwd)\"\n>>> +\t) &&\n>>> +\texpect_rejected_dir\n>>> +'\n>> \n>> I am debating myself if we want to remove the in-repository\n>> safe.directory configuration setting after this test piece is done,\n>> with test_when_finished. We just made sure, with this test, that\n>> having the variable does not affect anything, so the subsequent\n>> tests should not care hence it is probably OK. On the other hand,\n>> to make sure those settings they make (e.g. setting it globally is\n>> what the next test we can see below does) is what affects their\n>> outcome, it removes one more thing we need to worry about if we\n>> clean after ourselves. I dunno.\n>\n> test_config would do the same, right? I think it automatically\n> does the test_when_finished for us.\n\nI thought it (specifically, anything depends on test_when_finished)\nhas trouble working well from inside a subprocess?\n"},{"id":"454634","messageId":"20220429190625.GA1626@szeder.dev","threadId":"57724","inReplyTo":"xmqq8rrndb6h.fsf@gitster.g","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-04-29T19:06:25Z","receivedAt":"2022-04-29T19:06:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> > On 4/27/2022 4:37 PM, Junio C Hamano wrote:\n> >> SZEDER Gábor writes:\n> >> \n> >>> According to the documentation 'safe.directory' \"is only respected\n> >>> when specified in a system or global config, not when it is specified\n> >>> in a repository config or via the command line option -c\n> >>> safe.directory=\".\n> >>>\n> >>> Add tests to check that 'safe.directory' in the repository config or\n> >>> on the command line is indeed ignored.\n> >>>\n> >>> Signed-off-by: SZEDER Gábor \n> >>> ---\n> >>> t/t0033-safe-directory.sh | 13 +++++++++++++\n> >>> 1 file changed, 13 insertions(+)\n> >>>\n> >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> >>> index 6f9680e8b0..82dac0eb93 100755\n> >>> --- a/t/t0033-safe-directory.sh\n> >>> +++ b/t/t0033-safe-directory.sh\n> >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '\n> >>> \texpect_rejected_dir\n> >>> '\n> >>> \n> >>> +test_expect_success 'ignoring safe.directory on the command line' '\n> >>> +\ttest_must_fail git -c safe.directory=\"$(pwd)\" status 2>err &&\n> >>> +\tgrep \"unsafe repository\" err\n> >>> +'\n> >>> +\n> >>> +test_expect_success 'ignoring safe.directory in repo config' '\n> >>> +\t(\n> >>> +\t\tunset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> >>> +\t\tgit config safe.directory \"$(pwd)\"\n> >>> +\t) &&\n> >>> +\texpect_rejected_dir\n> >>> +'\n> >> \n> >> I am debating myself if we want to remove the in-repository\n> >> safe.directory configuration setting after this test piece is done,\n> >> with test_when_finished. We just made sure, with this test, that\n> >> having the variable does not affect anything, so the subsequent\n> >> tests should not care hence it is probably OK. On the other hand,\n> >> to make sure those settings they make (e.g. setting it globally is\n> >> what the next test we can see below does) is what affects their\n> >> outcome, it removes one more thing we need to worry about if we\n> >> clean after ourselves. I dunno.\n> >\n> > test_config would do the same, right? I think it automatically\n> > does the test_when_finished for us.\n> \n> I thought it (specifically, anything depends on test_when_finished)\n> has trouble working well from inside a subprocess?\n\nYeah, test_config doesn't work in a subshell, because modifying\nthe variable holding the cleanup commands won't be visible after\nleaving the subshell, and the protection added in 0968f12a99\n(test-lib-functions: detect test_when_finished in subshell,\n2015-09-05) will kick in. And we do need a subshell to set the\nconfig, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git\nconfig' would refuse to touch the config file.\n\nI think something like\n\n test_when_finished \"(\n unset GIT_TEST_ASSUME_DIFFERENT_USER &&\n git config --unset safe.directory\n )\"\n\nwould work, though.\n\nI considered cleaning up the config file, but all subsequent tests\nleave behind config settings for later tests as well (in the global\nconfig file, though).\n\n"},{"id":"454635","messageId":"b6deba3f-9019-fb6f-7bb3-cc2f647dcff6@github.com","threadId":"57724","inReplyTo":"20220429190625.GA1626@szeder.dev","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-29T19:19:01Z","receivedAt":"2022-04-29T19:19:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/29/2022 3:06 PM, SZEDER Gábor wrote:\n> On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:\n>> Derrick Stolee writes:\n>>> test_config would do the same, right? I think it automatically\n>>> does the test_when_finished for us.\n>>\n>> I thought it (specifically, anything depends on test_when_finished)\n>> has trouble working well from inside a subprocess?\n> \n> Yeah, test_config doesn't work in a subshell, because modifying\n> the variable holding the cleanup commands won't be visible after\n> leaving the subshell, and the protection added in 0968f12a99\n> (test-lib-functions: detect test_when_finished in subshell,\n> 2015-09-05) will kick in. And we do need a subshell to set the\n> config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git\n> config' would refuse to touch the config file.\n\nAh yes, of course.\n \n> I think something like\n> \n> test_when_finished \"(\n> unset GIT_TEST_ASSUME_DIFFERENT_USER &&\n> git config --unset safe.directory\n> )\"\n> \n> would work, though.\n\nWould it be simpler to use this?\n\n\tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n\nThanks,\n-Stolee\n"},{"id":"455056","messageId":"20220509213915.GA2043@szeder.dev","threadId":"57724","inReplyTo":"xmqqee1il09v.fsf@gitster.g","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-05-09T21:39:15Z","receivedAt":"2022-05-09T21:39:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like\n> > $PATH does), that would have been absolutely necessary to document\n> > how it works, but GIT_CONFIG_* is merely an implementation detail of\n> > how \"git -c var=val\" works and I am not sure if it is even a good\n> > idea to hardcode how they happen to work like these tests. The only\n> > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are\n> > used internally by the implementation and they should not muck with\n> > it, no?\n> \n> I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user\n> scripts, but then ...\n> \n> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\n> > index 6d764fe0cc..ae0e2e3bdb 100644\n> > --- a/Documentation/config/safe.txt\n> > +++ b/Documentation/config/safe.txt\n> > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a\n> > `safe.directory` entry with an empty value.\n> > +\n> > This config setting is only respected when specified in a system or global\n> > -config, not when it is specified in a repository config or via the command\n> > -line option `-c safe.directory=`.\n> > +config, not when it is specified in a repository config, via the command\n> > +line option `-c safe.directory=`, or in environment variables.\n> \n> ... this part must clarify what environment variables it is talking\n> about.\n> \n> ... via the command line option `-c safe.directory=`, or\n> via the GIT_CONFIG_{KEY,VALUE} mechanism.\n\nWell, the proposed phrasing covers all environment variables that\naffect the configuration.\n\nIt doesn't feel right to explicitly list all those variables,\nincluding GIT_CONFIG_PARAMETERS, because that vairable is such an\nimplementation detail that it isn't even mentioned elsewhere in the\ndocumentation at all. OTOH, listing only some of the ignored\nvariables but omitting others doesn't feel quite right either, hence\nthe general \"in environment variables\".\n\n> or something, perhaps. I actually do think it is a useful addition\n> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be\n> ignored.\n\nIs it safe to have such an environment variable?\n\nSo, it's quite clear why 'safe.directory' is ignored in the repository\nconfig: it can be under control of someone else, and thus a malicious\nrepositoy could trivially safe-list itself. However, it's unclear (to\nme) why 'git -c safe.directory=...' is ignored: 8959555cee\n(setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02) only states that it's ignored, but doesn't\njustify why. Now, let's suppose that there was a compelling reason to\ndo so, e.g. in some way that I don't readily see it can be spoofed and\nthus could be abused by an attacker to safe-list a malicious\nrepository. If that were indeed the case, then couldn't\n'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused\njust as easily?\n\nOr to approach it from the opposite direction: if such a\nGIT_SAFE_DIRECTORIES environment variable is safe, then why should we\nignore 'safe.directory' in the command line, or in the environment\nfor that matter?\n\n"},{"id":"455058","messageId":"xmqqlevabcsu.fsf@gitster.g","threadId":"57724","inReplyTo":"20220509213915.GA2043@szeder.dev","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-09T21:45:21Z","receivedAt":"2022-05-09T21:45:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> repositoy could trivially safe-list itself. However, it's unclear (to\n> me) why 'git -c safe.directory=...' is ignored: 8959555cee\n> (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02) only states that it's ignored, but doesn't\n> justify why. Now, let's suppose that there was a compelling reason to\n\nCorrect. I do not think it is a restriction with any sensible\njustification, just that it happened to be implemented that way.\n\nIOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user\ninterface than fixing the \"we ignore 'git -c safe.directory'?\" bug.\n"},{"id":"455060","messageId":"YnmVRoOpfvON6lwW@nand.local","threadId":"57724","inReplyTo":"20220427170649.4949-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T22:27:18Z","receivedAt":"2022-05-09T22:27:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:\n> - it is interpreted as a regexp by 'grep', so, because of the dot,\n> it matches the name of the test script and the path of the trash\n> directory as well. Consequently, these tests could be fooled by\n> any error message that would happen to include the path of the\n> test repository.\n\nNice catch. I wouldn't have even thought about \"could match the test\ndirectory\" as a potential way for this test to generate a false\npositive. So I'm happy to see us tightening this down.\n\nThanks.\n\nTaylor\n"},{"id":"455074","messageId":"20220510060453.d6t5q5lqssvba6tg@carlos-mbp.lan","threadId":"57724","inReplyTo":"20220427170649.4949-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-10T06:04:53Z","receivedAt":"2022-05-10T06:05:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Apologies for the late feedback, hoping it is still timely though.\n\nOn Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:\n> \n> - it only appears in the advice part, not in the actual error\n> message.\n> \n> - it is interpreted as a regexp by 'grep', so, because of the dot,\n> it matches the name of the test script and the path of the trash\n> directory as well. Consequently, these tests could be fooled by\n> any error message that would happen to include the path of the\n> test repository.\n\nThe subject says something else and I think it is confusing because the\nchange you propose only seems to be removing the path as an implementation\ndetail while the core of the change seems to be making sure that the text\nthat is being matched is a \"Fixed String\" and part of the real error.\n\nI agree is an improvement, but I think it could be done better by :\n\n* using `grep -F` instead to make sure we don't interpret that string\n as a regex by mistake.\n* including the path, so we are sure we really matched the error message\n that was really expected. As a side effect we also validate this way\n that the code that does the lookup and reports the \"name\" of the\n repository that could be added to the exception list, reports the righ\n directory, which seems by itself an important thing we should want to\n avoid regressing against in future changes.\n\nBoth changed are implemented by the patch below and I think might be\nworth considering as a replacement of this one.\n\nCarlo\n\nPS. Commit message could be improved, but wasn't sure how in the context\n of your series, so left it as a barebones one hoping it would be part\n of the patch you could adopt into your series.\n\n----- >8 -----\nSubject: t0033: verify detected worktree is accurate and part of the error\n \nChange the helper function to check for a fixed message and make\nsure to include the directory that we are testing from (which in\nall current tests is the current directory).\n \nSigned-off-by: Carlo Marcelo Arenas Belón \n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 239d93f4d2..c524b74b01 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER\n \n expect_rejected_dir () {\n \ttest_must_fail git status 2>err &&\n-\tgrep \"safe.directory\" err\n+\tgrep -F \"unsafe repository ('$(pwd)'\" err\n }\n \n test_expect_success 'safe.directory is not set' '\n"},{"id":"455101","messageId":"20220510183321.GA4408@szeder.dev","threadId":"57724","inReplyTo":"b6deba3f-9019-fb6f-7bb3-cc2f647dcff6@github.com","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-05-10T18:33:21Z","receivedAt":"2022-05-10T18:33:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:\n> > And we do need a subshell to set the\n> > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git\n> > config' would refuse to touch the config file.\n> \n> Ah yes, of course.\n> \n> > I think something like\n> > \n> > test_when_finished \"(\n> > unset GIT_TEST_ASSUME_DIFFERENT_USER &&\n> > git config --unset safe.directory\n> > )\"\n> > \n> > would work, though.\n> \n> Would it be simpler to use this?\n> \n> \tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n\nOh, wow. This is so obvious, no wonder it didn't occur to me :)\n\n"},{"id":"455103","messageId":"20220510184815.GB4408@szeder.dev","threadId":"57724","inReplyTo":"xmqqlevabcsu.fsf@gitster.g","subject":"Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-05-10T18:48:15Z","receivedAt":"2022-05-10T18:49:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, May 09, 2022 at 02:45:21PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > repositoy could trivially safe-list itself. However, it's unclear (to\n> > me) why 'git -c safe.directory=...' is ignored: 8959555cee\n> > (setup_git_directory(): add an owner check for the top-level\n> > directory, 2022-03-02) only states that it's ignored, but doesn't\n> > justify why. Now, let's suppose that there was a compelling reason to\n> \n> Correct. I do not think it is a restriction with any sensible\n> justification, just that it happened to be implemented that way.\n> \n> IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user\n> interface than fixing the \"we ignore 'git -c safe.directory'?\" bug.\n\nIn light of this, I'm not sure we want to test said buggy behavior.\nIOW, do we want 2/3 and 3/3 of this patch series at all? Or perhaps\nadd those test as 'test_expect_failure' instead?\n\n"},{"id":"455113","messageId":"YnrDK0wCV6taY1QX@nand.local","threadId":"57724","inReplyTo":"20220510183321.GA4408@szeder.dev","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-10T19:55:23Z","receivedAt":"2022-05-10T19:55:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:\n> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:\n> > > And we do need a subshell to set the\n> > > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git\n> > > config' would refuse to touch the config file.\n> >\n> > Ah yes, of course.\n> >\n> > > I think something like\n> > >\n> > > test_when_finished \"(\n> > > unset GIT_TEST_ASSUME_DIFFERENT_USER &&\n> > > git config --unset safe.directory\n> > > )\"\n> > >\n> > > would work, though.\n> >\n> > Would it be simpler to use this?\n> >\n> > \tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n>\n> Oh, wow. This is so obvious, no wonder it didn't occur to me :)\n\nDon't we consider this one-shot environment variable to be sticky on\nsome shells (i.e., that it persists beyond just the \"git config\"\ninvocation here)?\n\nThanks,\nTaylor\n"},{"id":"455114","messageId":"20220510200658.n2ythd5soyzguqb4@carlos-mbp.lan","threadId":"57724","inReplyTo":"YnrDK0wCV6taY1QX@nand.local","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-10T20:06:58Z","receivedAt":"2022-05-10T20:07:05Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:\n> > >\n> > > \tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n> >\n> > Oh, wow. This is so obvious, no wonder it didn't occur to me :)\n> \n> Don't we consider this one-shot environment variable to be sticky on\n> some shells (i.e., that it persists beyond just the \"git config\"\n> invocation here)?\n\ndo you have an example of such a shell?, I would assume that since the\nmechanism to implement these would be similar to local and we already\nrequire local for running our tests, that shouldn't be an issue (at\nleast in the test suite), right?\n\nany such variables should be only set as part of the environment used\nby the posix shell before it call execve to invoke the next command IMHO.\n\nCarlo\n"},{"id":"455115","messageId":"YnrHDieIQqjBUl/2@nand.local","threadId":"57724","inReplyTo":"20220510200658.n2ythd5soyzguqb4@carlos-mbp.lan","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-10T20:11:58Z","receivedAt":"2022-05-10T20:12:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 10, 2022 at 01:06:58PM -0700, Carlo Marcelo Arenas Belón wrote:\n> On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:\n> > > >\n> > > > \tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n> > >\n> > > Oh, wow. This is so obvious, no wonder it didn't occur to me :)\n> >\n> > Don't we consider this one-shot environment variable to be sticky on\n> > some shells (i.e., that it persists beyond just the \"git config\"\n> > invocation here)?\n>\n> do you have an example of such a shell?, I would assume that since the\n> mechanism to implement these would be similar to local and we already\n> require local for running our tests, that shouldn't be an issue (at\n> least in the test suite), right?\n>\n> any such variables should be only set as part of the environment used\n> by the posix shell before it call execve to invoke the next command IMHO.\n\nThis is completely my mistake, that stickiness exists only when invoking\nshell _functions_, not other commands (like \"git\").\n\nI have gotten so used to looking for the former, that I didn't read\ncarefully enough to realize that we are in the latter situation instead.\n\n> Carlo\n\nThanks,\nTaylor\n"},{"id":"455116","messageId":"69897c46-abe7-f4d0-b43a-f60e0215927c@sunshineco.com","threadId":"57724","inReplyTo":"YnrDK0wCV6taY1QX@nand.local","subject":"Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-05-10T20:18:34Z","receivedAt":"2022-05-10T20:18:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On 5/10/22 3:55 PM, Taylor Blau wrote:\n> On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:\n>> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:\n>>> Would it be simpler to use this?\n>>>\n>>> \tGIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory\n>>\n>> Oh, wow. This is so obvious, no wonder it didn't occur to me :)\n> \n> Don't we consider this one-shot environment variable to be sticky on\n> some shells (i.e., that it persists beyond just the \"git config\"\n> invocation here)?\n\nAlmost. Some old/buggy shells incorrectly allow the one-shot environment \nvariable assignment to bleed past command invocation when the \"command\" \nis a shell function, like this:\n\n SOME_ENV_VAR=somevalue shell_function_call\n\nWe do \"lint\"[*] for that sort of problem in t/check-non-portable-shell.pl.\n\nStolee's example, however, is invoking a normal program (`git`, in this \ncase), so isn't subject to that unwanted behavior from buggy shells.\n\n\n[*]: a0a630192d (t/check-non-portable-shell: detect \"FOO=bar \nshell_func\", 2018-07-13)\n"}]}