@matheusbsbs / matheus-bagliano / commits / 6a67c75948

Fork 0

test-lib-functions: restrict test_must_fail usage

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Denton Liu committed Jul 7, 2020 at 02:04 UTC 6a67c759489e1025665adf78326e9e0d0981bab5

2 files changed +65

t/t0000-basic.sh

+18

@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '

1271 test $len = 4098

1272 '

1273

1274 +test_expect_success 'test_must_fail on a failing git command' '

1275 + test_must_fail git notacommand

1276 +'

1277 +

1278 +test_expect_success 'test_must_fail on a failing git command with env' '

1279 + test_must_fail env var1=a var2=b git notacommand

1280 +'

1281 +

1282 +test_expect_success 'test_must_fail rejects a non-git command' '

1283 + ! test_must_fail grep ^$ notafile 2>err &&

1284 + grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err

1285 +'

1286 +

1287 +test_expect_success 'test_must_fail rejects a non-git command with env' '

1288 + ! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&

1289 + grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err

1290 +'

1291 +

1292 test_done

t/test-lib-functions.sh

+47

@@ -798,6 +798,37 @@ list_contains () {

798 return 1

799 }

800

801 +# Returns success if the arguments indicate that a command should be

802 +# accepted by test_must_fail(). If the command is run with env, the env

803 +# and its corresponding variable settings will be stripped before we

804 +# test the command being run.

805 +test_must_fail_acceptable () {

806 + if test "$1" = "env"

807 + then

808 + shift

809 + while test $# -gt 0

810 + do

811 + case "$1" in

812 + *?=*)

813 + shift

814 + ;;

815 + *)

816 + break

817 + ;;

818 + esac

819 + done

820 + fi

821 +

822 + case "$1" in

823 + git|__git*|test-tool|test-svn-fe|test_terminal)

824 + return 0

825 + ;;

826 + *)

827 + return 1

828 + ;;

829 + esac

830 +}

831 +

832 # This is not among top-level (test_expect_success | test_expect_failure)

833 # but is a prefix that can be used in the test script, like:

834 #

@@ -817,6 +848,17 @@ list_contains () {

848 # Multiple signals can be specified as a comma separated list.

849 # Currently recognized signal names are: sigpipe, success.

850 # (Don't use 'success', use 'test_might_fail' instead.)

851 +#

852 +# Do not use this to run anything but "git" and other specific testable

853 +# commands (see test_must_fail_acceptable()). We are not in the

854 +# business of vetting system supplied commands -- in other words, this

855 +# is wrong:

856 +#

857 +# test_must_fail grep pattern output

858 +#

859 +# Instead use '!':

860 +#

861 +# ! grep pattern output

862

863 test_must_fail () {

864 case "$1" in

@@ -828,6 +870,11 @@ test_must_fail () {

870 _test_ok=

871 ;;

872 esac

873 + if ! test_must_fail_acceptable "$@"

874 + then

875 + echo >&7 "test_must_fail: only 'git' is allowed: $*"

876 + return 1

877 + fi

878 "$@" 2>&7

879 exit_code=$?

880 if test $exit_code -eq 0 && ! list_contains "$_test_ok" success