git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
@ 2017-09-25 19:55 Stefan Beller
  2017-09-25 20:04 ` Jonathan Nieder
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Beller @ 2017-09-25 19:55 UTC (permalink / raw)
  To: git; +Cc: jrnieder, Stefan Beller

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" &&
+	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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-25 19:55 [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules Stefan Beller
@ 2017-09-25 20:04 ` Jonathan Nieder
  2017-09-25 22:50   ` Stefan Beller
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Nieder @ 2017-09-25 20:04 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

Stefan Beller wrote:

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-25 20:04 ` Jonathan Nieder
@ 2017-09-25 22:50   ` Stefan Beller
  2017-09-26  0:01     ` Jonathan Nieder
  2017-09-26  5:37     ` Johannes Sixt
  0 siblings, 2 replies; 10+ messages in thread
From: Stefan Beller @ 2017-09-25 22:50 UTC (permalink / raw)
  To: jrnieder; +Cc: git, sbeller

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-25 22:50   ` Stefan Beller
@ 2017-09-26  0:01     ` Jonathan Nieder
  2017-09-26  5:37     ` Johannes Sixt
  1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Nieder @ 2017-09-26  0:01 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

Stefan Beller wrote:

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-25 22:50   ` Stefan Beller
  2017-09-26  0:01     ` Jonathan Nieder
@ 2017-09-26  5:37     ` Johannes Sixt
  2017-09-26  6:28       ` Junio C Hamano
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2017-09-26  5:37 UTC (permalink / raw)
  To: Stefan Beller; +Cc: jrnieder, git

Am 26.09.2017 um 00:50 schrieb Stefan Beller:
> 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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-26  5:37     ` Johannes Sixt
@ 2017-09-26  6:28       ` Junio C Hamano
  2017-09-26 18:54         ` Stefan Beller
  0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2017-09-26  6:28 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Stefan Beller, jrnieder, git

Johannes Sixt <j6t@kdbg.org> writes:

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


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-26  6:28       ` Junio C Hamano
@ 2017-09-26 18:54         ` Stefan Beller
  2017-09-26 19:46           ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Beller @ 2017-09-26 18:54 UTC (permalink / raw)
  To: gitster; +Cc: git, j6t, jrnieder, sbeller

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

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-26 18:54         ` Stefan Beller
@ 2017-09-26 19:46           ` Johannes Sixt
  2017-09-26 19:54             ` Stefan Beller
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2017-09-26 19:46 UTC (permalink / raw)
  To: Stefan Beller; +Cc: gitster, git, jrnieder

Am 26.09.2017 um 20:54 schrieb Stefan Beller:
> +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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-26 19:46           ` Johannes Sixt
@ 2017-09-26 19:54             ` Stefan Beller
  2017-09-27  3:21               ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Beller @ 2017-09-26 19:54 UTC (permalink / raw)
  To: j6t; +Cc: git, gitster, jrnieder, sbeller

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

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules
  2017-09-26 19:54             ` Stefan Beller
@ 2017-09-27  3:21               ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2017-09-27  3:21 UTC (permalink / raw)
  To: Stefan Beller; +Cc: j6t, git, jrnieder

Stefan Beller <sbeller@google.com> writes:

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-27  3:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-25 19:55 [PATCH] t7406: submodule.<name>.update command must not be run from .gitmodules Stefan Beller
2017-09-25 20:04 ` Jonathan Nieder
2017-09-25 22:50   ` Stefan Beller
2017-09-26  0:01     ` Jonathan Nieder
2017-09-26  5:37     ` Johannes Sixt
2017-09-26  6:28       ` Junio C Hamano
2017-09-26 18:54         ` Stefan Beller
2017-09-26 19:46           ` Johannes Sixt
2017-09-26 19:54             ` Stefan Beller
2017-09-27  3:21               ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).