threads / patch / 46831

patcht7406: submodule.<name>.update command must not be run from .gitmodules

Subject: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

## tl;dr

10 messages between Sep 25, 2017 and Sep 27, 2017. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Stefan Beller· Sep 25, 2017, 19:55 UTC · lore

submodule.<name>.update can be assigned an arbitrary command via setting it to "!command". When this command is found in the regular config, Git ought to just run that command instead of other update mechanisms.

However if that command is just found in the .gitmodules file, it is potentially untrusted, which is why we do not run it. Add a test confirming the behavior.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to t/t7406-submodule-update.sh +10 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 034914a14f..780af4e6f5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '
 	)
 '
 
+test_expect_success 'submodule update - command in .gitmodules is ignored' '
+	test_when_finished "git -C super reset --hard HEAD^" &&
+
+	git -C super config -f .gitmodules submodule.submodule.update "!false || echo >bad" &&
+	git -C super commit -a -m "add command to .gitmodules file" &&
+	git -C super/submodule reset --hard $submodulesha1^ &&
+	git -C super submodule update submodule 2>../actual &&
+	test_path_is_missing super/bad
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 'submodule'
 EOF

-- 
2.14.0.rc0.3.g6c2e499285
Jonathan Nieder· Sep 25, 2017, 20:04 UTC · re: Stefan Beller · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Stefan Beller wrote:
Show 26 quoted lines
> submodule.<name>.update can be assigned an arbitrary command via setting
> it to "!command". When this command is found in the regular config, Git
> ought to just run that command instead of other update mechanisms.
> 
> However if that command is just found in the .gitmodules file, it is
> potentially untrusted, which is why we do not run it.  Add a test
> confirming the behavior.
> 
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7406-submodule-update.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 034914a14f..780af4e6f5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '
>  	)
>  '
>  
> +test_expect_success 'submodule update - command in .gitmodules is ignored' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
> +
> +	git -C super config -f .gitmodules submodule.submodule.update "!false || echo >bad" &&
What does the '!false || echo >bad' do?

Ideally we want this test to be super robust: e.g. if it runs the command but from a different directory, we still want the test to fail, and if it runs the command but using exec instead of a shell, we still want the test to fail.

Maybe write_script would help with this.  E.g. would something like
	test_when_finished ... &&
	write_script must_not_run.sh <<-EOF &&
	>$TEST_DIRECTORY/bad
	EOF
	git -C super config -f .gitmodules submodule.submodule.update \
		"!$TEST_DIRECTORY/must_not_run.sh" &&
	...
work?

Thanks, Jonathan

Stefan Beller· Sep 25, 2017, 22:50 UTC · re: Jonathan Nieder · lore

submodule.<name>.update can be assigned an arbitrary command via setting it to "!command". When this command is found in the regular config, Git ought to just run that command instead of other update mechanisms.

However if that command is just found in the .gitmodules file, it is potentially untrusted, which is why we do not run it. Add a test confirming the behavior.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 updated to use the super robust script.
 Thanks Jonathan,
 
 Stefan
 t/t7406-submodule-update.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to t/t7406-submodule-update.sh +14 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 034914a14f..d718cb00e7 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '
 	)
 '
 
+test_expect_success 'submodule update - command in .gitmodules is ignored' '
+	test_when_finished "git -C super reset --hard HEAD^" &&
+
+	write_script must_not_run.sh <<-EOF &&
+	>$TEST_DIRECTORY/bad
+	EOF
+
+	git -C super config -f .gitmodules submodule.submodule.update "!$TEST_DIRECTORY/must_not_run.sh" &&
+	git -C super commit -a -m "add command to .gitmodules file" &&
+	git -C super/submodule reset --hard $submodulesha1^ &&
+	git -C super submodule update submodule &&
+	test_path_is_missing bad
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 'submodule'
 EOF

-- 
2.14.0.rc0.3.g6c2e499285
Jonathan Nieder· Sep 26, 2017, 00:01 UTC · re: Stefan Beller · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Stefan Beller wrote:
Show 30 quoted lines
> submodule.<name>.update can be assigned an arbitrary command via setting
> it to "!command". When this command is found in the regular config, Git
> ought to just run that command instead of other update mechanisms.
>
> However if that command is just found in the .gitmodules file, it is
> potentially untrusted, which is why we do not run it.  Add a test
> confirming the behavior.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7406-submodule-update.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 034914a14f..d718cb00e7 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '
>  	)
>  '
>  
> +test_expect_success 'submodule update - command in .gitmodules is ignored' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
> +
> +	write_script must_not_run.sh <<-EOF &&
> +	>$TEST_DIRECTORY/bad
> +	EOF
> +
> +	git -C super config -f .gitmodules submodule.submodule.update "!$TEST_DIRECTORY/must_not_run.sh" &&

Long line, but I don't think I care. I wish there were a tool like "make style" to format shell scripts.

Show 5 quoted lines
> +	git -C super commit -a -m "add command to .gitmodules file" &&
> +	git -C super/submodule reset --hard $submodulesha1^ &&
> +	git -C super submodule update submodule &&
> +	test_path_is_missing bad
> +'

Per offline discussion, you tested that this fails when you use .git/config instead of .gitmodules, so there aren't any subtle typos here. :)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for writing it.
Johannes Sixt· Sep 26, 2017, 05:37 UTC · re: Stefan Beller · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Am 26.09.2017 um 00:50 schrieb Stefan Beller:
Show 34 quoted lines
> submodule.<name>.update can be assigned an arbitrary command via setting
> it to "!command". When this command is found in the regular config, Git
> ought to just run that command instead of other update mechanisms.
> 
> However if that command is just found in the .gitmodules file, it is
> potentially untrusted, which is why we do not run it.  Add a test
> confirming the behavior.
> 
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>   updated to use the super robust script.
>   Thanks Jonathan,
>   
>   Stefan
> 
>   t/t7406-submodule-update.sh | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 034914a14f..d718cb00e7 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '
>   	)
>   '
>   
> +test_expect_success 'submodule update - command in .gitmodules is ignored' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
> +
> +	write_script must_not_run.sh <<-EOF &&
> +	>$TEST_DIRECTORY/bad
> +	EOF

I am pretty confident that this does not test what you intend to test. Notice that $TEST_DIRECTORY is expanded when the script is written. But that path contains a blank, and we have something like this in the test script:

	#!/bin/sh
	>/the/build/directory/t/trash directory.t7406/bad

If you inject the bug against which this test protects into git-submodule, you should find a file "trash" in your t directory, and the file "bad" still absent. Not to mention that the script fails because it cannot run "directory.t7406/bad".

To fix that, you should use and exported variable and access that from the test script, for example:

	write_script must_not_run.sh <<-\EOF &&
	>"$TEST_DIRECTORY"/bad
	EOF
...
	(
		export TEST_DIRECTORY &&
		git -C super submodule update submodule
	) &&
	test_path_is_missing bad
Show 12 quoted lines
> +
> +	git -C super config -f .gitmodules submodule.submodule.update "!$TEST_DIRECTORY/must_not_run.sh" &&
> +	git -C super commit -a -m "add command to .gitmodules file" &&
> +	git -C super/submodule reset --hard $submodulesha1^ &&
> +	git -C super submodule update submodule &&
> +	test_path_is_missing bad
> +'
> +
>   cat << EOF >expect
>   Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>   EOF
> 
-- Hannes
Junio C Hamano· Sep 26, 2017, 06:28 UTC · re: Johannes Sixt · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Johannes Sixt <j6t@kdbg.org> writes:
Show 13 quoted lines
>> +	test_when_finished "git -C super reset --hard HEAD^" &&
>> +
>> +	write_script must_not_run.sh <<-EOF &&
>> +	>$TEST_DIRECTORY/bad
>> +	EOF
>
> I am pretty confident that this does not test what you intend to
> test. Notice that $TEST_DIRECTORY is expanded when the script is
> written. But that path contains a blank, and we have something like
> this in the test script:
>
> 	#!/bin/sh
> 	>/the/build/directory/t/trash directory.t7406/bad
Nicely explained.  Thanks.
Stefan Beller· Sep 26, 2017, 18:54 UTC · re: Junio C Hamano · lore

submodule.<name>.update can be assigned an arbitrary command via setting it to "!command". When this command is found in the regular config, Git ought to just run that command instead of other update mechanisms.

However if that command is just found in the .gitmodules file, it is potentially untrusted, which is why we do not run it. Add a test confirming the behavior.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Johannes wrote:
Show 7 quoted lines
> I am pretty confident that this does not test what you intend to
> test. Notice that $TEST_DIRECTORY is expanded when the script is
> written. But that path contains a blank, and we have something like
> this in the test script:
>
>       #!/bin/sh
>       >/the/build/directory/t/trash directory.t7406/bad
I can confirm that.

Instead of mucking around with writing a script, "to make it robust", I decided to got the simplest route and just have the command "!false", which would make "git submodule update submodule" return an error.

Thanks, Stefan

 t/t7406-submodule-update.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to t/t7406-submodule-update.sh +10 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 034914a14f..a9ea098e55 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '
 	)
 '
 
+test_expect_success 'submodule update - command in .gitmodules is ignored' '
+	test_when_finished "git -C super reset --hard HEAD^" &&
+
+	git -C super config -f .gitmodules submodule.submodule.update "!false" &&
+	git -C super commit -a -m "add command to .gitmodules file" &&
+	git -C super/submodule reset --hard $submodulesha1^ &&
+	git -C super submodule update submodule &&
+	test_path_is_missing bad
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 'submodule'
 EOF

-- 
2.14.0.rc0.3.g6c2e499285
Johannes Sixt· Sep 26, 2017, 19:46 UTC · re: Stefan Beller · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Am 26.09.2017 um 20:54 schrieb Stefan Beller:
Show 8 quoted lines
> +test_expect_success 'submodule update - command in .gitmodules is ignored' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
> +
> +	git -C super config -f .gitmodules submodule.submodule.update "!false" &&
> +	git -C super commit -a -m "add command to .gitmodules file" &&
> +	git -C super/submodule reset --hard $submodulesha1^ &&
> +	git -C super submodule update submodule &&
> +	test_path_is_missing bad

This test for a missing file is certainly a remnant from the previous iteration, isn't it?

-- Hannes
Stefan Beller· Sep 26, 2017, 19:54 UTC · re: Johannes Sixt · lore

submodule.<name>.update can be assigned an arbitrary command via setting it to "!command". When this command is found in the regular config, Git ought to just run that command instead of other update mechanisms.

However if that command is just found in the .gitmodules file, it is potentially untrusted, which is why we do not run it. Add a test confirming the behavior.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
> This test for a missing file is certainly a remnant from the
> previous iteration, isn't it?
Yes. This is a good indicator I need some vacation.

Thanks, Stefan

 t/t7406-submodule-update.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/t7406-submodule-update.sh +8 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 034914a14f..6f083c4d68 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -406,6 +406,14 @@ test_expect_success 'submodule update - command in .git/config' '
 	)
 '
 
+test_expect_success 'submodule update - command in .gitmodules is ignored' '
+	test_when_finished "git -C super reset --hard HEAD^" &&
+	git -C super config -f .gitmodules submodule.submodule.update "!false" &&
+	git -C super commit -a -m "add command to .gitmodules file" &&
+	git -C super/submodule reset --hard $submodulesha1^ &&
+	git -C super submodule update submodule
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 'submodule'
 EOF

-- 
2.14.0.rc0.3.g6c2e499285
Junio C Hamano· Sep 27, 2017, 03:21 UTC · re: Stefan Beller · lore

Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules

Stefan Beller <sbeller@google.com> writes:
Show 11 quoted lines
> submodule.<name>.update can be assigned an arbitrary command via setting
> it to "!command". When this command is found in the regular config, Git
> ought to just run that command instead of other update mechanisms.
>
> However if that command is just found in the .gitmodules file, it is
> potentially untrusted, which is why we do not run it.  Add a test
> confirming the behavior.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Earlier, we saw:
    Ideally we want this test to be super robust: e.g. if it runs the
    command but from a different directory, we still want the test to fail,
    and if it runs the command but using exec instead of a shell, we still
    want the test to fail.

and this one (i.e. signal that it is a command by prefixing with '!', and then have a single command that would fail whether it is run via run_command() with or without shell) would satisfy that criteria, I would think.

>> This test for a missing file is certainly a remnant from the
>> previous iteration, isn't it?
>
> Yes. This is a good indicator I need some vacation.

Or just take a deep breath before making a knee-jerk reaction public and instead double-check before sending things out ;-)

Will queue.  Thanks.
Show 26 quoted lines
>
> Thanks,
> Stefan
>
>  t/t7406-submodule-update.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 034914a14f..6f083c4d68 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -406,6 +406,14 @@ test_expect_success 'submodule update - command in .git/config' '
>  	)
>  '
>  
> +test_expect_success 'submodule update - command in .gitmodules is ignored' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
> +	git -C super config -f .gitmodules submodule.submodule.update "!false" &&
> +	git -C super commit -a -m "add command to .gitmodules file" &&
> +	git -C super/submodule reset --hard $submodulesha1^ &&
> +	git -C super submodule update submodule
> +'
> +
>  cat << EOF >expect
>  Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>  EOF

← back to recent threads