149 lines
6.1 KiB
Diff
149 lines
6.1 KiB
Diff
From e904deb89d9a9669a76a426182506a084d3f6308 Mon Sep 17 00:00:00 2001
|
|
From: Jonathan Nieder <jrnieder@gmail.com>
|
|
Date: Thu, 5 Dec 2019 01:28:28 -0800
|
|
Subject: [PATCH] submodule: reject submodule.update = !command in .gitmodules
|
|
|
|
Since ac1fbbda2013 (submodule: do not copy unknown update mode from
|
|
.gitmodules, 2013-12-02), Git has been careful to avoid copying
|
|
|
|
[submodule "foo"]
|
|
update = !run an arbitrary scary command
|
|
|
|
from .gitmodules to a repository's local config, copying in the
|
|
setting 'update = none' instead. The gitmodules(5) manpage documents
|
|
the intention:
|
|
|
|
The !command form is intentionally ignored here for security
|
|
reasons
|
|
|
|
Unfortunately, starting with v2.20.0-rc0 (which integrated ee69b2a9
|
|
(submodule--helper: introduce new update-module-mode helper,
|
|
2018-08-13, first released in v2.20.0-rc0)), there are scenarios where
|
|
we *don't* ignore it: if the config store contains no
|
|
submodule.foo.update setting, the submodule-config API falls back to
|
|
reading .gitmodules and the repository-supplied !command gets run
|
|
after all.
|
|
|
|
This was part of a general change over time in submodule support to
|
|
read more directly from .gitmodules, since unlike .git/config it
|
|
allows a project to change values between branches and over time
|
|
(while still allowing .git/config to override things). But it was
|
|
never intended to apply to this kind of dangerous configuration.
|
|
|
|
The behavior change was not advertised in ee69b2a9's commit message
|
|
and was missed in review.
|
|
|
|
Let's take the opportunity to make the protection more robust, even in
|
|
Git versions that are technically not affected: instead of quietly
|
|
converting 'update = !command' to 'update = none', noisily treat it as
|
|
an error. Allowing the setting but treating it as meaning something
|
|
else was just confusing; users are better served by seeing the error
|
|
sooner. Forbidding the construct makes the semantics simpler and
|
|
means we can check for it in fsck (in a separate patch).
|
|
|
|
As a result, the submodule-config API cannot read this value from
|
|
.gitmodules under any circumstance, and we can declare with confidence
|
|
|
|
For security reasons, the '!command' form is not accepted
|
|
here.
|
|
|
|
Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
|
|
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
|
|
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
|
|
---
|
|
Documentation/gitmodules.txt | 5 ++---
|
|
submodule-config.c | 10 +++++++++-
|
|
t/t7406-submodule-update.sh | 14 ++++++++------
|
|
3 files changed, 19 insertions(+), 10 deletions(-)
|
|
|
|
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
|
|
index a66e95b..f260b55 100644
|
|
--- a/Documentation/gitmodules.txt
|
|
+++ b/Documentation/gitmodules.txt
|
|
@@ -44,9 +44,8 @@ submodule.<name>.update::
|
|
submodule init` to initialize the configuration variable of
|
|
the same name. Allowed values here are 'checkout', 'rebase',
|
|
'merge' or 'none'. See description of 'update' command in
|
|
- linkgit:git-submodule[1] for their meaning. Note that the
|
|
- '!command' form is intentionally ignored here for security
|
|
- reasons.
|
|
+ linkgit:git-submodule[1] for their meaning. For security
|
|
+ reasons, the '!command' form is not accepted here.
|
|
|
|
submodule.<name>.branch::
|
|
A remote branch name for tracking updates in the upstream submodule.
|
|
diff --git a/submodule-config.c b/submodule-config.c
|
|
index 4264ee2..3acc6ad 100644
|
|
--- a/submodule-config.c
|
|
+++ b/submodule-config.c
|
|
@@ -405,6 +405,13 @@ struct parse_config_parameter {
|
|
int overwrite;
|
|
};
|
|
|
|
+/*
|
|
+ * Parse a config item from .gitmodules.
|
|
+ *
|
|
+ * This does not handle submodule-related configuration from the main
|
|
+ * config store (.git/config, etc). Callers are responsible for
|
|
+ * checking for overrides in the main config store when appropriate.
|
|
+ */
|
|
static int parse_config(const char *var, const char *value, void *data)
|
|
{
|
|
struct parse_config_parameter *me = data;
|
|
@@ -482,7 +489,8 @@ static int parse_config(const char *var, const char *value, void *data)
|
|
warn_multiple_config(me->treeish_name, submodule->name,
|
|
"update");
|
|
else if (parse_submodule_update_strategy(value,
|
|
- &submodule->update_strategy) < 0)
|
|
+ &submodule->update_strategy) < 0 ||
|
|
+ submodule->update_strategy.type == SM_UPDATE_COMMAND)
|
|
die(_("invalid value for %s"), var);
|
|
} else if (!strcmp(item.buf, "shallow")) {
|
|
if (!me->overwrite && submodule->recommend_shallow != -1)
|
|
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
|
|
index c973278..031ae85 100755
|
|
--- a/t/t7406-submodule-update.sh
|
|
+++ b/t/t7406-submodule-update.sh
|
|
@@ -407,12 +407,12 @@ test_expect_success 'submodule update - command in .git/config' '
|
|
)
|
|
'
|
|
|
|
-test_expect_success 'submodule update - command in .gitmodules is ignored' '
|
|
+test_expect_success 'submodule update - command in .gitmodules is rejected' '
|
|
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_must_fail git -C super submodule update submodule
|
|
'
|
|
|
|
cat << EOF >expect
|
|
@@ -481,6 +481,9 @@ test_expect_success 'recursive submodule update - command in .git/config catches
|
|
'
|
|
|
|
test_expect_success 'submodule init does not copy command into .git/config' '
|
|
+ test_when_finished "git -C super update-index --force-remove submodule1" &&
|
|
+ test_when_finished git config -f super/.gitmodules \
|
|
+ --remove-section submodule.submodule1 &&
|
|
(cd super &&
|
|
git ls-files -s submodule >out &&
|
|
H=$(cut -d" " -f2 out) &&
|
|
@@ -489,10 +492,9 @@ test_expect_success 'submodule init does not copy command into .git/config' '
|
|
git config -f .gitmodules submodule.submodule1.path submodule1 &&
|
|
git config -f .gitmodules submodule.submodule1.url ../submodule &&
|
|
git config -f .gitmodules submodule.submodule1.update !false &&
|
|
- git submodule init submodule1 &&
|
|
- echo "none" >expect &&
|
|
- git config submodule.submodule1.update >actual &&
|
|
- test_cmp expect actual
|
|
+ test_must_fail git submodule init submodule1 &&
|
|
+ test_expect_code 1 git config submodule.submodule1.update >actual &&
|
|
+ test_must_be_empty actual
|
|
)
|
|
'
|
|
|
|
--
|
|
1.8.3.1
|
|
|