Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

From: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Date: Jun 6, 2016, 11:39 UTC
Message-ID: <998280064.353519.1465213163063.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <20160604145101.21928-1-Antoine.Queru@grenoble-inp.org>

Hi Antoine,

Antoine Queru <Antoine.Queru@grenoble-inp.org> writes:

Show 12 quoted lines

> [...]
> +For example, if we set up the configuration variables like this:
> +
> +-------------------------------
> +git config --add remote.pushBlacklist repository.com
> +git config --add remote.pushWhitelist repository.com/Special_Folder
> +-------------------------------
> +
> +Pushes like this  will be accepted:
> +-------------------------------
> +git push repository.com/Special_Path/*
> +-------------------------------

According to your previous `git config`, it should be:
	git push repository.com/Special_Folder/*

Show 14 quoted lines

> +
> +While this one for example will be denied:
> +-------------------------------
> +git push repository.com/Other_Path/
> +-------------------------------
> +
> [...]
> +/*
> + *NEEDSWORK: Ugly because file://... is recognized as an url, and we
> + *may want to compare it to local path without this scheme. Forcing
> + *the user to put file:// before every local path would make the code
> + *easier and avoid confusion with a distant repo like 'github.com'
> + *which is not an url.
> + */

Style: space after '*' (when there is text after), meaning:
	/*
	 * NEEDSWORK: Ugly because file://... is recognized [...]
	 * [...]
	 */

> +static int longest_prefix_size(const char* target_str,
> +                                const struct string_list *list)

That might just be my mailer but this line is not properly lined up
with the previous one (one space too much).
It should be:
static int longest_prefix_size(const char* target_str,
			       const struct string_list *list)

Show 8 quoted lines

> [...]
> +        for_each_string_list_item(curr_item, list) {
> +                struct url_info curr_url;
> +                const char *curr_str = curr_item->string;
> +                skip_prefix(curr_str, "file://", &curr_str);
> +                url_normalize(curr_str, &curr_url);
> +                if (target_url.url &&
> +                    curr_url.url &&

You can put target_url.url and curr_url.url on the same line.

> +                    target_url.scheme_len == curr_url.scheme_len &&
> +                    !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))

Style: space after ','.

With those two things, the condition would look like this:

		if (target_url.url && curr_url.url &&
		    target_url.scheme_len == curr_url.scheme_len &&
		    !strncmp(target_url.url, curr_url.url, curr_url.scheme_len))

Show 5 quoted lines

> [...]
> +        whitelist_size = longest_prefix_size(repo, whitelist);
> +        blacklist_size = longest_prefix_size(repo, blacklist);
> +
> +        check_length_prefix(whitelist_size, blacklist_size, repo, deny_message, default_policy);

This line is above 80 characters, so:

	check_length_prefix(whitelist_size, blacklist_size, repo, deny_message,
			    default_policy);

Show 11 quoted lines

> [...]
> +test_expect_success 'setup' '
> +        git init --bare blacklist/ &&
> +        git init --bare whitelist/ &&
> +        git init --bare blacklist/allow &&
> +        test_commit commit &&
> +        echo "fatal: Pushing to this remote using this protocol is forbidden" > forbidden
> +'
> +
> +test_expect_success 'basic case' '
> +        git config --add remote.pushBlacklist http://blacklist.com &&

You use `git config` instead of `test_config`, meaning that the configuration you introduce will persist after the test.

It is not really a problem here since for the other tests you don't use `git config --add` so the configuration will be overwritten. However I still think you should use `test_config` to avoid causing trouble to potential future tests that would use `--add` and expect a clean state.

Show 6 quoted lines

> [...]
> +test_expect_success 'local path with file://' '
> +        git config remote.pushBlacklist file://blacklist &&
> +        test_must_fail git push blacklist HEAD 2> result &&
> +        test_cmp result forbidden
> +'

(you forgot a new line here)

Show 8 quoted lines

> +test_expect_success 'only one scheme allowed' '
> +        git config remote.pushDefaultPolicy deny &&
> +        git config remote.pushWhitelist http://blacklist.com &&
> +        test_must_fail git push https://blacklist.com HEAD 2> result &&
> +        test_cmp result forbidden
> +'
> +
> +test_expect_success 'denied repo in allowed repo' '

'allowed repo in denied remote'? In any case the current title is misleading for me.

> +        git config remote.pushBlacklist blacklist &&
> +        git config --add remote.pushWhitelist blacklist/allow &&
> +        git push blacklist/allow HEAD
> +'

Thanks, Rémi