Re: [PATCH] urlmatch: do not allow passwords in URLs by default

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 1, 2021, 08:44 UTC
Message-ID: <87fsz6ygyc.fsf@evledraar.gmail.com>
In-Reply-To: <pull.945.git.1619807844627.gitgitgadget@gmail.com>

On Fri, Apr 30 2021, Derrick Stolee via GitGitGadget wrote:

Just nits on the patch, will reply to the idea in another message:

> [...]
> +test_expect_success 'enable username:password urls' '
> +	git config --global core.allowUsernamePasswordUrls true
> +'

Hrm, --global? In any case isn't it also better here to tweak this for specific tests?

Show 7 quoted lines

>  test_expect_success 'push status output scrubs password' '
>  	cd "$ROOT_PATH/test_repo_clone" &&
> +	git config core.allowUsernamePasswordUrls true &&
>  	git push --porcelain \
>  		"$HTTPD_URL_USER_PASS/smart/test_repo.git" \
>  		+HEAD:scrub >status &&
> @@ -469,9 +470,11 @@ test_expect_success 'push status output scrubs password' '

Use test_config instead, unless this is really "setup for the rest of the tests" in disguise, but IMO even more of a reason to use test_config for each one.

Show 8 quoted lines

>  test_expect_success 'clone/fetch scrubs password from reflogs' '
>  	cd "$ROOT_PATH" &&
> -	git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" \
> +	git -c core.allowUsernamePasswordUrls=true clone \
> +		"$HTTPD_URL_USER_PASS/smart/test_repo.git" \
>  		reflog-test &&
>  	cd reflog-test &&
> +	git config core.allowUsernamePasswordUrls true &&

Ditto. Although redundant in your patch, no, since we've set it to true above?

Show 5 quoted lines

> +test_expect_success 'clone fails when using username:password' '
> +	test_must_fail git clone https://username:password@bogus.url 2>err &&
> +	test_i18ngrep "attempted to parse a URL with a plain-text username and password" err
> +'
> +

Just use grep, not test_i18ngrep. GETTEXT_POISON is gone.

Show 24 quoted lines

>  test_expect_success 'clone from hooks' '
>  
>  	test_create_repo r0 &&
> diff --git a/urlmatch.c b/urlmatch.c
> index 33a2ccd306b6..e81ec9e1fc0b 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -1,5 +1,6 @@
>  #include "cache.h"
>  #include "urlmatch.h"
> +#include "config.h"
>  
>  #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
>  #define URL_DIGIT "0123456789"
> @@ -106,6 +107,18 @@ static int match_host(const struct url_info *url_info,
>  	return (!url_len && !pat_len);
>  }
>  
> +static void die_if_username_password_not_allowed(void)
> +{
> +	int opt_in = 0;
> +	if (!git_config_get_bool("core.allowusernamepasswordurls", &opt_in) &&
> +	    opt_in)
> +		return;

API use nit: You need to either initialize "opt_in = 0" or check the git_config_get_bool() return value. Doing both isn't strictly needed. I.e. either of these would work:

    int opt_in = 0;
    git_config_get_bool(..., &opt_in);
    if (opt_in) ...;

Or:

    int opt_in;
    if (!git_config_get_bool(..., &opt_in) && opt_in)

No?