[RFC,2/2] upload-pack.c: allow banning certain object filter(s)

Commit Message

Taylor Blau March 17, 2020, 8:39 p.m. UTC

Git clients may ask the server for a partial set of objects, where the
set of objects being requested is refined by one or more object filters.
Server administrators can configure 'git upload-pack' to allow or ban
these filters by setting the 'uploadpack.allowFilter' variable to
'true' or 'false', respectively.

However, administrators using bitmaps may wish to allow certain kinds of
object filters, but ban others. Specifically, they may wish to allow
object filters that can be optimized by the use of bitmaps, while
rejecting other object filters which aren't and represent a perceived
performance degradation (as well as an increased load factor on the
server).

Allow configuring 'git upload-pack' to support object filters on a
case-by-case basis by introducing a new configuration variable and
section:

  - 'uploadpack.filter.allow'

  - 'uploadpack.filter.<kind>.allow'

where '<kind>' may be one of 'blob:none', 'blob:limit', 'tree:depth',
and so on. The additional '.' between 'filter' and '<kind>' is part of
the sub-section.

Setting the second configuration variable for any valid value of
'<kind>' explicitly allows or disallows restricting that kind of object
filter.

If a client requests the object filter <kind> and the respective
configuration value is not set, 'git upload-pack' will default to the
value of 'uploadpack.filter.allow', which itself defaults to 'true' to
maintain backwards compatibility. Note that this differs from
'uploadpack.allowfilter', which controls whether or not the 'filter'
capability is advertised.

NB: this introduces an unfortunate possibility that attempt to write the
ERR sideband will cause a SIGPIPE. This can be prevented by some of
SZEDZER's previous work, but it is silenced in 't' for now.
---
 Documentation/config/uploadpack.txt | 12 ++++++
 t/t5616-partial-clone.sh            | 23 ++++++++++
 upload-pack.c                       | 67 +++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)

Comments

Eric Sunshine March 17, 2020, 9:11 p.m. UTC | #1

On Tue, Mar 17, 2020 at 4:40 PM Taylor Blau <[e-mail]> wrote:
> NB: this introduces an unfortunate possibility that attempt to write the
> ERR sideband will cause a SIGPIPE. This can be prevented by some of
> SZEDZER's previous work, but it is silenced in 't' for now.

s/SZEDZER/SZEDER/

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> @@ -235,6 +235,29 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
> +test_expect_success 'upload-pack fails banned object filters' '
> +       # Ensure that configuration keys are normalized by capitalizing
> +       # "blob:None" below:
> +       test_config -C srv.bare uploadpack.filter.blob:None.allow false &&

I found the wording of the comment more confusing than clarifying.
Perhaps rewriting it like this could help:

    Test case-insensitivity by intentional use of "blob:None" rather than
    "blob:none".

or something.

> +       test_must_fail ok=sigpipe git clone --no-checkout --filter.blob:none \
> +               "file://$(pwd)/srv.bare" pc3
> +'

Philip Oakley March 18, 2020, 11:18 a.m. UTC | #2

Hi
On 17/03/2020 20:39, Taylor Blau wrote:
> Git clients may ask the server for a partial set of objects, where the
> set of objects being requested is refined by one or more object filters.
> Server administrators can configure 'git upload-pack' to allow or ban
> these filters by setting the 'uploadpack.allowFilter' variable to
> 'true' or 'false', respectively.
>
> However, administrators using bitmaps may wish to allow certain kinds of
> object filters, but ban others. Specifically, they may wish to allow
> object filters that can be optimized by the use of bitmaps, while
> rejecting other object filters which aren't and represent a perceived
> performance degradation (as well as an increased load factor on the
> server).
>
> Allow configuring 'git upload-pack' to support object filters on a
> case-by-case basis by introducing a new configuration variable and
> section:
>
>   - 'uploadpack.filter.allow'
>
>   - 'uploadpack.filter.<kind>.allow'
>
> where '<kind>' may be one of 'blob:none', 'blob:limit', 'tree:depth',
> and so on. The additional '.' between 'filter' and '<kind>' is part of
> the sub-section.
>
> Setting the second configuration variable for any valid value of
> '<kind>' explicitly allows or disallows restricting that kind of object
> filter.
>
> If a client requests the object filter <kind> and the respective
> configuration value is not set, 'git upload-pack' will default to the
> value of 'uploadpack.filter.allow', which itself defaults to 'true' to
> maintain backwards compatibility. Note that this differs from
> 'uploadpack.allowfilter', which controls whether or not the 'filter'
> capability is advertised.
>
> NB: this introduces an unfortunate possibility that attempt to write the
> ERR sideband will cause a SIGPIPE. This can be prevented by some of
> SZEDZER's previous work, but it is silenced in 't' for now.
> ---
>  Documentation/config/uploadpack.txt | 12 ++++++
>  t/t5616-partial-clone.sh            | 23 ++++++++++
>  upload-pack.c                       | 67 +++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>
> diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
> index ed1c835695..6213bd619c 100644
> --- a/Documentation/config/uploadpack.txt
> +++ b/Documentation/config/uploadpack.txt
> @@ -57,6 +57,18 @@ uploadpack.allowFilter::
>  	If this option is set, `upload-pack` will support partial
>  	clone and partial fetch object filtering.
>  
> +uploadpack.filter.allow::
> +	Provides a default value for unspecified object filters (see: the
> +	below configuration variable).
> +	Defaults to `true`.
> +
> +uploadpack.filter.<filter>.allow::
> +	Explicitly allow or ban the object filter corresponding to `<filter>`,
> +	where `<filter>` may be one of: `blob:none`, `blob:limit`, `tree:depth`,
> +	`sparse:oid`, or `combine`. If using combined filters, both `combine`
> +	and all of the nested filter kinds must be allowed.

Doesn't the man page at least need the part from the commit message "The
additional '.' between 'filter' and '<kind>' is part of
the sub-section." as it's not a common mechanism (other comments not
withstanding)

Philip
> +	Defaults to `uploadpack.filter.allow`.
> +
>  uploadpack.allowRefInWant::
>  	If this option is set, `upload-pack` will support the `ref-in-want`
>  	feature of the protocol version 2 `fetch` command.  This feature
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 77bb91e976..ee1af9b682 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -235,6 +235,29 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
>  	test_cmp unique_types.expected unique_types.actual
>  '
>  
> +test_expect_success 'upload-pack fails banned object filters' '
> +	# Ensure that configuration keys are normalized by capitalizing
> +	# "blob:None" below:
> +	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter.blob:none \
> +		"file://$(pwd)/srv.bare" pc3
> +'
> +
> +test_expect_success 'upload-pack fails banned combine object filters' '
> +	test_config -C srv.bare uploadpack.filter.allow false &&
> +	test_config -C srv.bare uploadpack.filter.combine.allow true &&
> +	test_config -C srv.bare uploadpack.filter.tree:depth.allow true &&
> +	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> +		--filter=blob:none "file://$(pwd)/srv.bare" pc3
> +'
> +
> +test_expect_success 'upload-pack fails banned object filters with fallback' '
> +	test_config -C srv.bare uploadpack.filter.allow false &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3
> +'
> +
>  test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
>  	rm -rf src dst &&
>  	git init src &&
> diff --git a/upload-pack.c b/upload-pack.c
> index c53249cac1..81f2701f99 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -69,6 +69,8 @@ static int filter_capability_requested;
>  static int allow_filter;
>  static int allow_ref_in_want;
>  static struct list_objects_filter_options filter_options;
> +static struct string_list allowed_filters = STRING_LIST_INIT_DUP;
> +static int allow_filter_fallback = 1;
>  
>  static int allow_sideband_all;
>  
> @@ -848,6 +850,45 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
>  	return 0;
>  }
>  
> +static int allows_filter_choice(enum list_objects_filter_choice c)
> +{
> +	const char *key = list_object_filter_config_name(c);
> +	struct string_list_item *item = string_list_lookup(&allowed_filters,
> +							   key);
> +	if (item)
> +		return (intptr_t) item->util;
> +	return allow_filter_fallback;
> +}
> +
> +static struct list_objects_filter_options *banned_filter(
> +	struct list_objects_filter_options *opts)
> +{
> +	size_t i;
> +
> +	if (!allows_filter_choice(opts->choice))
> +		return opts;
> +
> +	if (opts->choice == LOFC_COMBINE)
> +		for (i = 0; i < opts->sub_nr; i++) {
> +			struct list_objects_filter_options *sub = &opts->sub[i];
> +			if (banned_filter(sub))
> +				return sub;
> +		}
> +	return NULL;
> +}
> +
> +static void die_if_using_banned_filter(struct packet_writer *w,
> +				       struct list_objects_filter_options *opts)
> +{
> +	struct list_objects_filter_options *banned = banned_filter(opts);
> +	if (!banned)
> +		return;
> +
> +	packet_writer_error(w, _("filter '%s' not supported\n"),
> +			    list_object_filter_config_name(banned->choice));
> +	die(_("git upload-pack: banned object filter requested"));
> +}
> +
>  static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)
>  {
>  	struct object_array shallows = OBJECT_ARRAY_INIT;
> @@ -885,6 +926,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
>  				die("git upload-pack: filtering capability not negotiated");
>  			list_objects_filter_die_if_populated(&filter_options);
>  			parse_list_objects_filter(&filter_options, arg);
> +			die_if_using_banned_filter(&writer, &filter_options);
>  			continue;
>  		}
>  
> @@ -1044,6 +1086,9 @@ static int find_symref(const char *refname, const struct object_id *oid,
>  
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
> +	const char *sub, *key;
> +	int sub_len;
> +
>  	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
>  		if (git_config_bool(var, value))
>  			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
> @@ -1065,6 +1110,26 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>  			keepalive = -1;
>  	} else if (!strcmp("uploadpack.allowfilter", var)) {
>  		allow_filter = git_config_bool(var, value);
> +	} else if (!parse_config_key(var, "uploadpack", &sub, &sub_len, &key) &&
> +		   key && !strcmp(key, "allow")) {
> +		if (sub && skip_prefix(sub, "filter.", &sub) && sub_len >= 7) {
> +			struct string_list_item *item;
> +			char *spec;
> +
> +			/*
> +			 * normalize the filter, and chomp off '.allow' from the
> +			 * end
> +			 */
> +			spec = xstrdup_tolower(sub);
> +			spec[sub_len - 7] = 0;
> +
> +			item = string_list_insert(&allowed_filters, spec);
> +			item->util = (void *) (intptr_t) git_config_bool(var, value);
> +
> +			free(spec);
> +		} else if (!strcmp("uploadpack.filter.allow", var)) {
> +			allow_filter_fallback = git_config_bool(var, value);
> +		}
>  	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
>  		allow_ref_in_want = git_config_bool(var, value);
>  	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
> @@ -1308,6 +1373,8 @@ static void process_args(struct packet_reader *request,
>  		if (allow_filter && skip_prefix(arg, "filter ", &p)) {
>  			list_objects_filter_die_if_populated(&filter_options);
>  			parse_list_objects_filter(&filter_options, p);
> +			die_if_using_banned_filter(&data->writer,
> +						   &filter_options);
>  			continue;
>  		}
>

Taylor Blau March 18, 2020, 9:18 p.m. UTC | #3

On Tue, Mar 17, 2020 at 05:11:42PM -0400, Eric Sunshine wrote:
> On Tue, Mar 17, 2020 at 4:40 PM Taylor Blau <[e-mail]> wrote:
> > NB: this introduces an unfortunate possibility that attempt to write the
> > ERR sideband will cause a SIGPIPE. This can be prevented by some of
> > SZEDZER's previous work, but it is silenced in 't' for now.
>
> s/SZEDZER/SZEDER/

Thank you for pointing this out, and my apologies to SZEDER.

> > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> > @@ -235,6 +235,29 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
> > +test_expect_success 'upload-pack fails banned object filters' '
> > +       # Ensure that configuration keys are normalized by capitalizing
> > +       # "blob:None" below:
> > +       test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
>
> I found the wording of the comment more confusing than clarifying.
> Perhaps rewriting it like this could help:
>
>     Test case-insensitivity by intentional use of "blob:None" rather than
>     "blob:none".
>
> or something.

Sure, your suggestion does clarify things. I'll apply it to my fork.

> > +       test_must_fail ok=sigpipe git clone --no-checkout --filter.blob:none \
> > +               "file://$(pwd)/srv.bare" pc3
> > +'

Thanks,
Taylor

Taylor Blau March 18, 2020, 9:20 p.m. UTC | #4

Hi Philip,

On Wed, Mar 18, 2020 at 11:18:16AM +0000, Philip Oakley wrote:
> Hi
> On 17/03/2020 20:39, Taylor Blau wrote:
> > Git clients may ask the server for a partial set of objects, where the
> > set of objects being requested is refined by one or more object filters.
> > Server administrators can configure 'git upload-pack' to allow or ban
> > these filters by setting the 'uploadpack.allowFilter' variable to
> > 'true' or 'false', respectively.
> >
> > However, administrators using bitmaps may wish to allow certain kinds of
> > object filters, but ban others. Specifically, they may wish to allow
> > object filters that can be optimized by the use of bitmaps, while
> > rejecting other object filters which aren't and represent a perceived
> > performance degradation (as well as an increased load factor on the
> > server).
> >
> > Allow configuring 'git upload-pack' to support object filters on a
> > case-by-case basis by introducing a new configuration variable and
> > section:
> >
> >   - 'uploadpack.filter.allow'
> >
> >   - 'uploadpack.filter.<kind>.allow'
> >
> > where '<kind>' may be one of 'blob:none', 'blob:limit', 'tree:depth',
> > and so on. The additional '.' between 'filter' and '<kind>' is part of
> > the sub-section.
> >
> > Setting the second configuration variable for any valid value of
> > '<kind>' explicitly allows or disallows restricting that kind of object
> > filter.
> >
> > If a client requests the object filter <kind> and the respective
> > configuration value is not set, 'git upload-pack' will default to the
> > value of 'uploadpack.filter.allow', which itself defaults to 'true' to
> > maintain backwards compatibility. Note that this differs from
> > 'uploadpack.allowfilter', which controls whether or not the 'filter'
> > capability is advertised.
> >
> > NB: this introduces an unfortunate possibility that attempt to write the
> > ERR sideband will cause a SIGPIPE. This can be prevented by some of
> > SZEDZER's previous work, but it is silenced in 't' for now.
> > ---
> >  Documentation/config/uploadpack.txt | 12 ++++++
> >  t/t5616-partial-clone.sh            | 23 ++++++++++
> >  upload-pack.c                       | 67 +++++++++++++++++++++++++++++
> >  3 files changed, 102 insertions(+)
> >
> > diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
> > index ed1c835695..6213bd619c 100644
> > --- a/Documentation/config/uploadpack.txt
> > +++ b/Documentation/config/uploadpack.txt
> > @@ -57,6 +57,18 @@ uploadpack.allowFilter::
> >  	If this option is set, `upload-pack` will support partial
> >  	clone and partial fetch object filtering.
> >
> > +uploadpack.filter.allow::
> > +	Provides a default value for unspecified object filters (see: the
> > +	below configuration variable).
> > +	Defaults to `true`.
> > +
> > +uploadpack.filter.<filter>.allow::
> > +	Explicitly allow or ban the object filter corresponding to `<filter>`,
> > +	where `<filter>` may be one of: `blob:none`, `blob:limit`, `tree:depth`,
> > +	`sparse:oid`, or `combine`. If using combined filters, both `combine`
> > +	and all of the nested filter kinds must be allowed.
>
> Doesn't the man page at least need the part from the commit message "The
> additional '.' between 'filter' and '<kind>' is part of
> the sub-section." as it's not a common mechanism (other comments not
> withstanding)

Thanks, you're certainly right. I wrote the man pages back when the
configuration was spelled:

  $ git config uploadpack.filter=blob:none.allow true

But now that there is the extra '.', it's worth calling out here, too.
I'll make sure that this is addressed based on the outcome of the
discussion below when these patches hit non-RFC status.

> Philip

Thanks,
Taylor

Patch

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..6213bd619c 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,18 @@  uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpack.filter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpack.filter.<filter>.allow::
+	Explicitly allow or ban the object filter corresponding to `<filter>`,
+	where `<filter>` may be one of: `blob:none`, `blob:limit`, `tree:depth`,
+	`sparse:oid`, or `combine`. If using combined filters, both `combine`
+	and all of the nested filter kinds must be allowed.
+	Defaults to `uploadpack.filter.allow`.
+
 uploadpack.allowRefInWant::
 	If this option is set, `upload-pack` will support the `ref-in-want`
 	feature of the protocol version 2 `fetch` command.  This feature
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 77bb91e976..ee1af9b682 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,29 @@  test_expect_success 'implicitly construct combine: filter with repeated flags' '
 	test_cmp unique_types.expected unique_types.actual
 '
 
+test_expect_success 'upload-pack fails banned object filters' '
+	# Ensure that configuration keys are normalized by capitalizing
+	# "blob:None" below:
+	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter.blob:none \
+		"file://$(pwd)/srv.bare" pc3
+'
+
+test_expect_success 'upload-pack fails banned combine object filters' '
+	test_config -C srv.bare uploadpack.filter.allow false &&
+	test_config -C srv.bare uploadpack.filter.combine.allow true &&
+	test_config -C srv.bare uploadpack.filter.tree:depth.allow true &&
+	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		--filter=blob:none "file://$(pwd)/srv.bare" pc3
+'
+
+test_expect_success 'upload-pack fails banned object filters with fallback' '
+	test_config -C srv.bare uploadpack.filter.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3
+'
+
 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
 	rm -rf src dst &&
 	git init src &&
diff --git a/upload-pack.c b/upload-pack.c
index c53249cac1..81f2701f99 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -69,6 +69,8 @@  static int filter_capability_requested;
 static int allow_filter;
 static int allow_ref_in_want;
 static struct list_objects_filter_options filter_options;
+static struct string_list allowed_filters = STRING_LIST_INIT_DUP;
+static int allow_filter_fallback = 1;
 
 static int allow_sideband_all;
 
@@ -848,6 +850,45 @@  static int process_deepen_not(const char *line, struct string_list *deepen_not,
 	return 0;
 }
 
+static int allows_filter_choice(enum list_objects_filter_choice c)
+{
+	const char *key = list_object_filter_config_name(c);
+	struct string_list_item *item = string_list_lookup(&allowed_filters,
+							   key);
+	if (item)
+		return (intptr_t) item->util;
+	return allow_filter_fallback;
+}
+
+static struct list_objects_filter_options *banned_filter(
+	struct list_objects_filter_options *opts)
+{
+	size_t i;
+
+	if (!allows_filter_choice(opts->choice))
+		return opts;
+
+	if (opts->choice == LOFC_COMBINE)
+		for (i = 0; i < opts->sub_nr; i++) {
+			struct list_objects_filter_options *sub = &opts->sub[i];
+			if (banned_filter(sub))
+				return sub;
+		}
+	return NULL;
+}
+
+static void die_if_using_banned_filter(struct packet_writer *w,
+				       struct list_objects_filter_options *opts)
+{
+	struct list_objects_filter_options *banned = banned_filter(opts);
+	if (!banned)
+		return;
+
+	packet_writer_error(w, _("filter '%s' not supported\n"),
+			    list_object_filter_config_name(banned->choice));
+	die(_("git upload-pack: banned object filter requested"));
+}
+
 static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -885,6 +926,7 @@  static void receive_needs(struct packet_reader *reader, struct object_array *wan
 				die("git upload-pack: filtering capability not negotiated");
 			list_objects_filter_die_if_populated(&filter_options);
 			parse_list_objects_filter(&filter_options, arg);
+			die_if_using_banned_filter(&writer, &filter_options);
 			continue;
 		}
 
@@ -1044,6 +1086,9 @@  static int find_symref(const char *refname, const struct object_id *oid,
 
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
+	const char *sub, *key;
+	int sub_len;
+
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
 		if (git_config_bool(var, value))
 			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
@@ -1065,6 +1110,26 @@  static int upload_pack_config(const char *var, const char *value, void *unused)
 			keepalive = -1;
 	} else if (!strcmp("uploadpack.allowfilter", var)) {
 		allow_filter = git_config_bool(var, value);
+	} else if (!parse_config_key(var, "uploadpack", &sub, &sub_len, &key) &&
+		   key && !strcmp(key, "allow")) {
+		if (sub && skip_prefix(sub, "filter.", &sub) && sub_len >= 7) {
+			struct string_list_item *item;
+			char *spec;
+
+			/*
+			 * normalize the filter, and chomp off '.allow' from the
+			 * end
+			 */
+			spec = xstrdup_tolower(sub);
+			spec[sub_len - 7] = 0;
+
+			item = string_list_insert(&allowed_filters, spec);
+			item->util = (void *) (intptr_t) git_config_bool(var, value);
+
+			free(spec);
+		} else if (!strcmp("uploadpack.filter.allow", var)) {
+			allow_filter_fallback = git_config_bool(var, value);
+		}
 	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
 		allow_ref_in_want = git_config_bool(var, value);
 	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
@@ -1308,6 +1373,8 @@  static void process_args(struct packet_reader *request,
 		if (allow_filter && skip_prefix(arg, "filter ", &p)) {
 			list_objects_filter_die_if_populated(&filter_options);
 			parse_list_objects_filter(&filter_options, p);
+			die_if_using_banned_filter(&data->writer,
+						   &filter_options);
 			continue;
 		}