Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> If the server did not advertise the capability to have signed pushes
> it should not accept signed pushes as stated in
> Documentation/technical/protocol-capabilities.txt:
>
>     Client will then send a space separated list of capabilities it wants
>     to be in effect. The client MUST NOT ask for capabilities the server
>     did not say it supports.
>
>     Server MUST diagnose and abort if capabilities it does not understand
>     was sent.  Server MUST NOT ignore capabilities that client requested
>     and server advertised.  As a consequence of these rules, server MUST
>     NOT advertise capabilities it does not understand.
>
> After rereading the second paragraph I think they should also be reworded to
>
>     Server MUST diagnose and abort if capabilities it did not advertise
>     was sent.

Except for s/was sent/was requested/, I think that rule makes sense
very much.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4c069c5..628d13a 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
>  				use_atomic = 1;
>  		}
>  
> -		if (!strcmp(line, "push-cert")) {
> +		if (push_cert_nonce &&
> +		    !strcmp(line, "push-cert")) {
>  			int true_flush = 0;
>  			char certbuf[1024];

This implementation is somewhat questionable.

The server knows how to parse "push-cert" line, knows that what
follows after that line up to "push-cert-end" line are shaped very
differently from protocol commands outside the push-cert block.  In
other words, it knows how to parse the request meant for the capable
server; it just wants to refuse to serve that request.

The patched code will make it fail by hoping that queue_command()
that only handles "40-hex 40-hex ref" will reject the line that
begins with "push-cert".  Instead of relying on such a hidden
dependency, wouldn't it be cleaner to actually parse the push-cert
block and then at the end notice and explictly say "Your requests
were syntactically correct, but I am not going to honor your request
to use the push-cert extension, because I never told you that I'd
offer you that capability", instead of rejecting the request with "I
was expecting old/new/ref but you sent a line with 'push-cert' on
it; what are you talking about?"