{"thread":{"id":"46831","subject":"[PATCH] t7406: submodule..update command must not be run from .gitmodules","startedAt":"2017-09-25T19:55:44Z","lastAt":"2017-09-27T03:21:37Z","messageCount":10,"participants":["Stefan Beller","Jonathan Nieder","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328870","messageId":"20170925195533.15172-1-sbeller@google.com","threadId":"46831","inReplyTo":null,"subject":"[PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-25T19:55:33Z","receivedAt":"2017-09-25T19:55:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"submodule..update can be assigned an arbitrary command via setting\nit to \"!command\". When this command is found in the regular config, Git\nought to just run that command instead of other update mechanisms.\n\nHowever if that command is just found in the .gitmodules file, it is\npotentially untrusted, which is why we do not run it. Add a test\nconfirming the behavior.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n t/t7406-submodule-update.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 034914a14f..780af4e6f5 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '\n \t)\n '\n \n+test_expect_success 'submodule update - command in .gitmodules is ignored' '\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n+\n+\tgit -C super config -f .gitmodules submodule.submodule.update \"!false || echo >bad\" &&\n+\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n+\tgit -C super/submodule reset --hard $submodulesha1^ &&\n+\tgit -C super submodule update submodule 2>../actual &&\n+\ttest_path_is_missing super/bad\n+'\n+\n cat << EOF >expect\n Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"328871","messageId":"20170925200448.GY27425@aiede.mtv.corp.google.com","threadId":"46831","inReplyTo":"20170925195533.15172-1-sbeller@google.com","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T20:04:48Z","receivedAt":"2017-09-25T20:04:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> submodule..update can be assigned an arbitrary command via setting\n> it to \"!command\". When this command is found in the regular config, Git\n> ought to just run that command instead of other update mechanisms.\n> \n> However if that command is just found in the .gitmodules file, it is\n> potentially untrusted, which is why we do not run it. Add a test\n> confirming the behavior.\n> \n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n> t/t7406-submodule-update.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 034914a14f..780af4e6f5 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule update - command in .gitmodules is ignored' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> +\n> +\tgit -C super config -f .gitmodules submodule.submodule.update \"!false || echo >bad\" &&\n\nWhat does the '!false || echo >bad' do?\n\nIdeally we want this test to be super robust: e.g. if it runs the\ncommand but from a different directory, we still want the test to fail,\nand if it runs the command but using exec instead of a shell, we still\nwant the test to fail.\n\nMaybe write_script would help with this. E.g. would something like\n\n\ttest_when_finished ... &&\n\twrite_script must_not_run.sh <<-EOF &&\n\t>$TEST_DIRECTORY/bad\n\tEOF\n\n\tgit -C super config -f .gitmodules submodule.submodule.update \\\n\t\t\"!$TEST_DIRECTORY/must_not_run.sh\" &&\n\t...\n\nwork?\n\nThanks,\nJonathan\n"},{"id":"328893","messageId":"20170925225054.10819-1-sbeller@google.com","threadId":"46831","inReplyTo":"20170925200448.GY27425@aiede.mtv.corp.google.com","subject":"[PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-25T22:50:54Z","receivedAt":"2017-09-25T22:51:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"submodule..update can be assigned an arbitrary command via setting\nit to \"!command\". When this command is found in the regular config, Git\nought to just run that command instead of other update mechanisms.\n\nHowever if that command is just found in the .gitmodules file, it is\npotentially untrusted, which is why we do not run it. Add a test\nconfirming the behavior.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n\n updated to use the super robust script.\n Thanks Jonathan,\n \n Stefan\n\n t/t7406-submodule-update.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 034914a14f..d718cb00e7 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '\n \t)\n '\n \n+test_expect_success 'submodule update - command in .gitmodules is ignored' '\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n+\n+\twrite_script must_not_run.sh <<-EOF &&\n+\t>$TEST_DIRECTORY/bad\n+\tEOF\n+\n+\tgit -C super config -f .gitmodules submodule.submodule.update \"!$TEST_DIRECTORY/must_not_run.sh\" &&\n+\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n+\tgit -C super/submodule reset --hard $submodulesha1^ &&\n+\tgit -C super submodule update submodule &&\n+\ttest_path_is_missing bad\n+'\n+\n cat << EOF >expect\n Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"328906","messageId":"20170926000121.GM27425@aiede.mtv.corp.google.com","threadId":"46831","inReplyTo":"20170925225054.10819-1-sbeller@google.com","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:01:21Z","receivedAt":"2017-09-26T00:01:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> submodule..update can be assigned an arbitrary command via setting\n> it to \"!command\". When this command is found in the regular config, Git\n> ought to just run that command instead of other update mechanisms.\n>\n> However if that command is just found in the .gitmodules file, it is\n> potentially untrusted, which is why we do not run it. Add a test\n> confirming the behavior.\n>\n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n> t/t7406-submodule-update.sh | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 034914a14f..d718cb00e7 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule update - command in .gitmodules is ignored' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> +\n> +\twrite_script must_not_run.sh <<-EOF &&\n> +\t>$TEST_DIRECTORY/bad\n> +\tEOF\n> +\n> +\tgit -C super config -f .gitmodules submodule.submodule.update \"!$TEST_DIRECTORY/must_not_run.sh\" &&\n\nLong line, but I don't think I care. I wish there were a tool like\n\"make style\" to format shell scripts.\n\n> +\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n> +\tgit -C super/submodule reset --hard $submodulesha1^ &&\n> +\tgit -C super submodule update submodule &&\n> +\ttest_path_is_missing bad\n> +'\n\nPer offline discussion, you tested that this fails when you use\n.git/config instead of .gitmodules, so there aren't any subtle typos\nhere. :)\n\nReviewed-by: Jonathan Nieder \n\nThanks for writing it.\n"},{"id":"328930","messageId":"0aa754b4-93fb-3f23-be51-4af5227ac847@kdbg.org","threadId":"46831","inReplyTo":"20170925225054.10819-1-sbeller@google.com","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-09-26T05:37:30Z","receivedAt":"2017-09-26T05:37:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.09.2017 um 00:50 schrieb Stefan Beller:\n> submodule..update can be assigned an arbitrary command via setting\n> it to \"!command\". When this command is found in the regular config, Git\n> ought to just run that command instead of other update mechanisms.\n> \n> However if that command is just found in the .gitmodules file, it is\n> potentially untrusted, which is why we do not run it. Add a test\n> confirming the behavior.\n> \n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n> \n> updated to use the super robust script.\n> Thanks Jonathan,\n> \n> Stefan\n> \n> t/t7406-submodule-update.sh | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n> \n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 034914a14f..d718cb00e7 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in .git/config' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule update - command in .gitmodules is ignored' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> +\n> +\twrite_script must_not_run.sh <<-EOF &&\n> +\t>$TEST_DIRECTORY/bad\n> +\tEOF\n\nI am pretty confident that this does not test what you intend to test. \nNotice that $TEST_DIRECTORY is expanded when the script is written. But \nthat path contains a blank, and we have something like this in the test \nscript:\n\n\t#!/bin/sh\n\t>/the/build/directory/t/trash directory.t7406/bad\n\nIf you inject the bug against which this test protects into \ngit-submodule, you should find a file \"trash\" in your t directory, and \nthe file \"bad\" still absent. Not to mention that the script fails \nbecause it cannot run \"directory.t7406/bad\".\n\nTo fix that, you should use and exported variable and access that from \nthe test script, for example:\n\n\twrite_script must_not_run.sh <<-\\EOF &&\n\t>\"$TEST_DIRECTORY\"/bad\n\tEOF\n...\n\t(\n\t\texport TEST_DIRECTORY &&\n\t\tgit -C super submodule update submodule\n\t) &&\n\ttest_path_is_missing bad\n\n> +\n> +\tgit -C super config -f .gitmodules submodule.submodule.update \"!$TEST_DIRECTORY/must_not_run.sh\" &&\n> +\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n> +\tgit -C super/submodule reset --hard $submodulesha1^ &&\n> +\tgit -C super submodule update submodule &&\n> +\ttest_path_is_missing bad\n> +'\n> +\n> cat << EOF >expect\n> Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> EOF\n> \n\n-- Hannes\n"},{"id":"328933","messageId":"xmqqshfa9doa.fsf@gitster.mtv.corp.google.com","threadId":"46831","inReplyTo":"0aa754b4-93fb-3f23-be51-4af5227ac847@kdbg.org","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-26T06:28:21Z","receivedAt":"2017-09-26T06:28:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n>> +\n>> +\twrite_script must_not_run.sh <<-EOF &&\n>> +\t>$TEST_DIRECTORY/bad\n>> +\tEOF\n>\n> I am pretty confident that this does not test what you intend to\n> test. Notice that $TEST_DIRECTORY is expanded when the script is\n> written. But that path contains a blank, and we have something like\n> this in the test script:\n>\n> \t#!/bin/sh\n> \t>/the/build/directory/t/trash directory.t7406/bad\n\nNicely explained. Thanks.\n\n"},{"id":"328957","messageId":"20170926185413.21209-1-sbeller@google.com","threadId":"46831","inReplyTo":"xmqqshfa9doa.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-26T18:54:13Z","receivedAt":"2017-09-26T18:54:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"submodule..update can be assigned an arbitrary command via setting\nit to \"!command\". When this command is found in the regular config, Git\nought to just run that command instead of other update mechanisms.\n\nHowever if that command is just found in the .gitmodules file, it is\npotentially untrusted, which is why we do not run it. Add a test\nconfirming the behavior.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n\nJohannes wrote:\n> I am pretty confident that this does not test what you intend to\n> test. Notice that $TEST_DIRECTORY is expanded when the script is\n> written. But that path contains a blank, and we have something like\n> this in the test script:\n>\n> #!/bin/sh\n> >/the/build/directory/t/trash directory.t7406/bad\n\nI can confirm that.\n\nInstead of mucking around with writing a script, \"to make it robust\",\nI decided to got the simplest route and just have the command \"!false\",\nwhich would make \"git submodule update submodule\" return an error.\n\nThanks,\nStefan\n\n\n t/t7406-submodule-update.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 034914a14f..a9ea098e55 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -406,6 +406,16 @@ test_expect_success 'submodule update - command in .git/config' '\n \t)\n '\n \n+test_expect_success 'submodule update - command in .gitmodules is ignored' '\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n+\n+\tgit -C super config -f .gitmodules submodule.submodule.update \"!false\" &&\n+\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n+\tgit -C super/submodule reset --hard $submodulesha1^ &&\n+\tgit -C super submodule update submodule &&\n+\ttest_path_is_missing bad\n+'\n+\n cat << EOF >expect\n Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"328962","messageId":"0b69c0b0-6246-2428-b4b0-bb3ef3cb5ae7@kdbg.org","threadId":"46831","inReplyTo":"20170926185413.21209-1-sbeller@google.com","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-09-26T19:46:50Z","receivedAt":"2017-09-26T19:47:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.09.2017 um 20:54 schrieb Stefan Beller:\n> +test_expect_success 'submodule update - command in .gitmodules is ignored' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> +\n> +\tgit -C super config -f .gitmodules submodule.submodule.update \"!false\" &&\n> +\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n> +\tgit -C super/submodule reset --hard $submodulesha1^ &&\n> +\tgit -C super submodule update submodule &&\n> +\ttest_path_is_missing bad\n\nThis test for a missing file is certainly a remnant from the previous \niteration, isn't it?\n\n-- Hannes\n"},{"id":"328963","messageId":"20170926195413.3916-1-sbeller@google.com","threadId":"46831","inReplyTo":"0b69c0b0-6246-2428-b4b0-bb3ef3cb5ae7@kdbg.org","subject":"[PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-26T19:54:13Z","receivedAt":"2017-09-26T19:54:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"submodule..update can be assigned an arbitrary command via setting\nit to \"!command\". When this command is found in the regular config, Git\nought to just run that command instead of other update mechanisms.\n\nHowever if that command is just found in the .gitmodules file, it is\npotentially untrusted, which is why we do not run it. Add a test\nconfirming the behavior.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n\n> This test for a missing file is certainly a remnant from the\n> previous iteration, isn't it?\n\nYes. This is a good indicator I need some vacation.\n\nThanks,\nStefan\n\n t/t7406-submodule-update.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 034914a14f..6f083c4d68 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -406,6 +406,14 @@ test_expect_success 'submodule update - command in .git/config' '\n \t)\n '\n \n+test_expect_success 'submodule update - command in .gitmodules is ignored' '\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n+\tgit -C super config -f .gitmodules submodule.submodule.update \"!false\" &&\n+\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n+\tgit -C super/submodule reset --hard $submodulesha1^ &&\n+\tgit -C super submodule update submodule\n+'\n+\n cat << EOF >expect\n Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"328995","messageId":"xmqqr2us7rns.fsf@gitster.mtv.corp.google.com","threadId":"46831","inReplyTo":"20170926195413.3916-1-sbeller@google.com","subject":"Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T03:21:27Z","receivedAt":"2017-09-27T03:21:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> submodule..update can be assigned an arbitrary command via setting\n> it to \"!command\". When this command is found in the regular config, Git\n> ought to just run that command instead of other update mechanisms.\n>\n> However if that command is just found in the .gitmodules file, it is\n> potentially untrusted, which is why we do not run it. Add a test\n> confirming the behavior.\n>\n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n\nEarlier, we saw:\n\n Ideally we want this test to be super robust: e.g. if it runs the\n command but from a different directory, we still want the test to fail,\n and if it runs the command but using exec instead of a shell, we still\n want the test to fail.\n\nand this one (i.e. signal that it is a command by prefixing with\n'!', and then have a single command that would fail whether it is\nrun via run_command() with or without shell) would satisfy that\ncriteria, I would think.\n\n>> This test for a missing file is certainly a remnant from the\n>> previous iteration, isn't it?\n>\n> Yes. This is a good indicator I need some vacation.\n\nOr just take a deep breath before making a knee-jerk reaction public\nand instead double-check before sending things out ;-)\n\nWill queue. Thanks.\n\n>\n> Thanks,\n> Stefan\n>\n> t/t7406-submodule-update.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 034914a14f..6f083c4d68 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -406,6 +406,14 @@ test_expect_success 'submodule update - command in .git/config' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule update - command in .gitmodules is ignored' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> +\tgit -C super config -f .gitmodules submodule.submodule.update \"!false\" &&\n> +\tgit -C super commit -a -m \"add command to .gitmodules file\" &&\n> +\tgit -C super/submodule reset --hard $submodulesha1^ &&\n> +\tgit -C super submodule update submodule\n> +'\n> +\n> cat << EOF >expect\n> Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> EOF\n"}]}