* [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).