Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to the safe.directory config option #1215

Closed
wants to merge 3 commits into from
Closed

Updates to the safe.directory config option #1215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b18bd1
t0033: add tests for safe.directory
derrickstolee Apr 13, 2022
8a06688
setup: fix safe.directory key not being checked
Matheus28 Apr 13, 2022
a5faf3a
setup: opt-out of check with safe.directory=*
derrickstolee Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Documentation/config/safe.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,10 @@ line option `-c safe.directory=<path>`.
The value of this setting is interpolated, i.e. `~/<path>` expands to a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

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

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

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

OK.

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

OK.

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

Thanks.

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.
12 changes: 9 additions & 3 deletions setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,9 +1100,14 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Matheus Valadares via GitGitGadget" <[email protected]>
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.

> 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(+)

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

struct safe_directory_data *data = d;

if (!value || !*value)
if (strcmp(key, "safe.directory"))
return 0;

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) &&
Expand All @@ -1119,7 +1124,8 @@ static int ensure_valid_ownership(const char *path)
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> 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.

OK.

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

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

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

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

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

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/13/2022 12:24 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> 
>> 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.
> 
> OK.
> 
>> -	if (is_path_owned_by_current_user(path))
>> +	if (is_path_owned_by_current_user(path) &&
>> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>>  		return 1;
> 
> Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
> real logic kicks in, I wonder?

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

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

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


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

> 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.

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

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

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

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

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

nit: $PWD instead of $(pwd)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Ævar Arnfjörð Bjarmason <[email protected]> writes:

(just this part)

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

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

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

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/13/2022 3:16 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <[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.
> 
> I think this is a good trade-off, but FWIW I'd think we could test also
> without the git_env_bool() by having the test depend on !NOT_ROOT, then
> check the owner of t/test-lib.sh, and chown to that user (i.e. the
> "real" user).
> 
> But that's all sorts of more fragile than just this test variable..
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> 
> Instead of this "export" perhaps just add it in front of the "git
> status"?

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

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

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

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

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

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

Thanks,
-Stolee

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);
Expand Down
49 changes: 49 additions & 0 deletions t/t0033-safe-directory.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!/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 '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
'

test_expect_success 'safe.directory matches, but is reset' '
git config --global --add safe.directory "" &&
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