From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage
Date: Wed, 1 Jul 2020 00:27:13 -0400 [thread overview]
Message-ID: <3ebbda6c57005967b718d834aed7e3eaf00d18b4.1593576602.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1593576601.git.liu.denton@gmail.com>
In previous commits, we removed the usage of test_must_fail() for most
commands except for a set of pre-approved commands. Since that's done,
only allow test_must_fail() to run those pre-approved commands.
Obviously, we should allow `git`.
We allow `__git*` as some completion functions return an error code that
comes from a git invocation. It's good to avoid using test_must_fail
unnecessarily but it wouldn't hurt to err on the side of caution when
we're potentially wrapping a git command (like in these cases).
We also allow `test-tool` and `test-svn-fe` because these are helper
commands that are written by us and we want to catch their failure.
Finally, we allow `test_terminal` because `test_terminal` just wraps
around git commands. Also, we cannot rewrite
`test_must_fail test_terminal` as `test_terminal test_must_fail` because
test_must_fail() is a shell function and as a result, it cannot be
invoked from the test-terminal Perl script.
We opted to explicitly list the above tools instead of using a catch-all
such as `test[-_]*` because we want to be as restrictive as possible so
that in the future, someone would not accidentally introduce an
unrelated usage of test_must_fail() on an "unapproved" command.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t0000-basic.sh | 18 ++++++++++++++++
t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2ff176cd5d..90bf1dbc8d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '
test $len = 4098
'
+test_expect_success 'test_must_fail on a failing git command' '
+ test_must_fail git notacommand
+'
+
+test_expect_success 'test_must_fail on a failing git command with env' '
+ test_must_fail env var1=a var2=b git notacommand
+'
+
+test_expect_success 'test_must_fail rejects a non-git command' '
+ ! test_must_fail grep ^$ notafile 2>err &&
+ grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+'
+
+test_expect_success 'test_must_fail rejects a non-git command with env' '
+ ! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&
+ grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+'
+
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3103be8a32..b791933ffd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -798,6 +798,37 @@ list_contains () {
return 1
}
+# Returns success if the arguments indicate that a command should be
+# accepted by test_must_fail(). If the command is run with env, the env
+# and its corresponding variable settings will be stripped before we
+# test the command being run.
+test_must_fail_acceptable () {
+ if test "$1" = "env"
+ then
+ shift
+ while test $# -gt 0
+ do
+ case "$1" in
+ *?=*)
+ shift
+ ;;
+ *)
+ break
+ ;;
+ esac
+ done
+ fi
+
+ case "$1" in
+ git|__git*|test-tool|test-svn-fe|test_terminal)
+ return 0
+ ;;
+ *)
+ return 1
+ ;;
+ esac
+}
+
# This is not among top-level (test_expect_success | test_expect_failure)
# but is a prefix that can be used in the test script, like:
#
@@ -817,6 +848,17 @@ list_contains () {
# Multiple signals can be specified as a comma separated list.
# Currently recognized signal names are: sigpipe, success.
# (Don't use 'success', use 'test_might_fail' instead.)
+#
+# Do not use this to run anything but "git" and other specific testable
+# commands (see test_must_fail_acceptable()). We are not in the
+# business of vetting system supplied commands -- in other words, this
+# is wrong:
+#
+# test_must_fail grep pattern output
+#
+# Instead use '!':
+#
+# ! grep pattern output
test_must_fail () {
case "$1" in
@@ -828,6 +870,11 @@ test_must_fail () {
_test_ok=
;;
esac
+ if ! test_must_fail_acceptable "$@"
+ then
+ echo >&7 "test_must_fail: only 'git' is allowed: $*"
+ return 1
+ fi
"$@" 2>&7
exit_code=$?
if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
--
2.27.0.383.g050319c2ae
next prev parent reply other threads:[~2020-07-01 4:27 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-30 15:03 [PATCH 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-06-30 15:03 ` [PATCH 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-06-30 21:48 ` Eric Sunshine
2020-06-30 22:06 ` Junio C Hamano
2020-06-30 15:03 ` [PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-06-30 15:03 ` [PATCH 3/5] t7107: don't use test_must_fail() Denton Liu
2020-06-30 15:03 ` [PATCH 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-06-30 15:03 ` [PATCH 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-06-30 20:56 ` Eric Sunshine
2020-06-30 21:59 ` Eric Sunshine
2020-06-30 23:39 ` Denton Liu
2020-07-01 4:27 ` [PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-01 4:27 ` [PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-01 4:27 ` [PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-01 4:27 ` [PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-01 4:27 ` [PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-01 4:27 ` Denton Liu [this message]
2020-07-07 6:04 ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-07 6:04 ` [RESEND PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-07 6:04 ` [RESEND PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-07 6:04 ` [RESEND PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-07 6:04 ` [RESEND PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-07 6:04 ` [RESEND PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-07-07 20:08 ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 20:57 ` Junio C Hamano
2020-07-07 22:08 ` [PATCH] t9400: don't use test_must_fail with cvs Denton Liu
2020-07-07 22:11 ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 22:21 ` Denton Liu
2020-07-07 22:29 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ebbda6c57005967b718d834aed7e3eaf00d18b4.1593576602.git.liu.denton@gmail.com \
--to=liu.denton@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).