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

PATCH In review Stefan Beller 4 participants 10 messages Sep 25, 2017 – Sep 27, 2017

Share & tools

Stefan Beller

author

2017-09-25 19:55 UTC #0 lore raw

PATCH 1/1 t7406: submodule.<name>.update command must not be run from .gitmodules

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.

Signed-off-by Stefan Beller <[email protected]>

---
 t/t7406-submodule-update.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

t/t7406-submodule-update.sh +10 -0

index 034914a14f..780af4e6f5 100755
@@ -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

Signature · 1 line

2.14.0.rc0.3.g6c2e499285

Jonathan Nieder

2017-09-25 20:04 UTC #1 lore raw

Quoted 19 lines · 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 <[email protected]>
Signed-off-by Stefan Beller <[email protected]>
---
 t/t7406-submodule-update.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)
t/t7406-submodule-update.sh +0 -0
index 034914a14f..780af4e6f5 100755

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 412

Quoted 7 lines

t/t7406-submodule-update.sh +4 -0

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

author

2017-09-25 22:51 UTC #2 lore raw

PATCH 1/1 t7406: submodule.<name>.update command must not be run from .gitmodules

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.

Signed-off-by Stefan Beller <[email protected]>

---

updated to use the super robust script.
Thanks Jonathan,

Stefan

 t/t7406-submodule-update.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

t/t7406-submodule-update.sh +14 -0

index 034914a14f..d718cb00e7 100755
@@ -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

Signature · 1 line

2.14.0.rc0.3.g6c2e499285

Jonathan Nieder

Reviewed-by on v1 1/1

2017-09-26 00:01 UTC #3 lore raw

Quoted 20 lines · 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 <[email protected]>
Signed-off-by Stefan Beller <[email protected]>
---
 t/t7406-submodule-update.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
t/t7406-submodule-update.sh +0 -0
index 034914a14f..d718cb00e7 100755
@@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '
 	)

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 416

Quoted 10 lines

t/t7406-submodule-update.sh +8 -0

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

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 421

Quoted 5 lines

t/t7406-submodule-update.sh +5 -0

+	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 <[email protected]>

Thanks for writing it.

Johannes Sixt

2017-09-26 05:37 UTC #4 lore raw

In reply to Stefan Beller

Am 26.09.2017 um 00:50 schrieb Stefan Beller:

Quoted 25 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 <[email protected]>
Signed-off-by Stefan Beller <[email protected]>
---
updated to use the super robust script.
Thanks Jonathan,
Stefan
  t/t7406-submodule-update.sh | 14 ++++++++++++++
  1 file changed, 14 insertions(+)
t/t7406-submodule-update.sh +0 -0
index 034914a14f..d718cb00e7 100755

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 414

Quoted 9 lines

t/t7406-submodule-update.sh +6 -0

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

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 425

Quoted 12 lines

t/t7406-submodule-update.sh +8 -0

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

2017-09-26 06:28 UTC #5 lore raw

Quoted 11 lines · Johannes Sixt

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 414
Quoted 5 lines
t/t7406-submodule-update.sh +5 -0
+	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:

Quoted 2 lines

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

Nicely explained. Thanks.

Stefan Beller

author

2017-09-26 18:54 UTC #6 lore raw

PATCH 1/1 t7406: submodule.<name>.update command must not be run from .gitmodules

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.

Signed-off-by Stefan Beller <[email protected]>

---

Quoted 7 lines · Johannes

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

t/t7406-submodule-update.sh +10 -0

index 034914a14f..a9ea098e55 100755
@@ -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

Signature · 1 line

2.14.0.rc0.3.g6c2e499285

Johannes Sixt

2017-09-26 19:47 UTC #7 lore raw

Am 26.09.2017 um 20:54 schrieb Stefan Beller:

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config' ' · line 416

Quoted 8 lines

t/t7406-submodule-update.sh +8 -0

+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

author

2017-09-26 19:54 UTC #8 lore raw

PATCH 1/1 t7406: submodule.<name>.update command must not be run from .gitmodules

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.

Signed-off-by Stefan Beller <[email protected]>

---

Quoted 2 lines

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

t/t7406-submodule-update.sh +8 -0

index 034914a14f..6f083c4d68 100755
@@ -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

Signature · 1 line

2.14.0.rc0.3.g6c2e499285

Junio C Hamano

2017-09-27 03:21 UTC #9 lore raw

Quoted 11 lines · 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 <[email protected]>
Signed-off-by Stefan Beller <[email protected]>
---

Earlier, we saw:

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.

Quoted 4 lines

Quoted 2 lines
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.

Quoted 26 lines

Thanks,
Stefan

 t/t7406-submodule-update.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

t/t7406-submodule-update.sh +8 -0

index 034914a14f..6f083c4d68 100755
@@ -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