Merge branch 'jt/push-options-doc'

The receive-pack program now makes sure that the push certificate
records the same set of push options used for pushing.

* jt/push-options-doc:
  receive-pack: verify push options in cert
  docs: correct receive.advertisePushOptions default

4 files changed, 114 insertions, 11 deletions

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 96e9cf8b73..e0b9fd0bc3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2623,9 +2623,8 @@ receive.advertiseAtomic::
 	capability, set this variable to false.
 
 receive.advertisePushOptions::
-	By default, git-receive-pack will advertise the push options
-	capability to its clients. If you don't want to advertise this
-	capability, set this variable to false.
+	When set to true, git-receive-pack will advertise the push options
+	capability to its clients. False by default.
 
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef20..a34917153f 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 ----
-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-requests   =  *shallow ( command-list | push-cert )
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*( VCHAR | SP )
+----
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+----
+  push-options      =  *PKT-LINE(push-option) flush-pkt
+----
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it MUST send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+MUST be the same, modulo the prefix.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+----
+  packfile          =  "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0bb36d584d..1b29975c52 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+			 const char **next_line)
 {
 	int key_len = strlen(key);
 	const char *line = msg;
@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
 		if (line + key_len < eol &&
 		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
 			int offset = key_len + 1;
+			if (next_line)
+				*next_line = *eol ? eol + 1 : eol;
 			return xmemdupz(line + offset, (eol - line) - offset);
 		}
 		line = *eol ? eol + 1 : NULL;
@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-	char *nonce = find_header(buf, len, "nonce");
+	char *nonce = find_header(buf, len, "nonce", NULL);
 	timestamp_t stamp, ostamp;
 	char *bohmac, *expect = NULL;
 	const char *retval = NONCE_BAD;
@@ -575,6 +578,45 @@ leave:
 	return retval;
 }
 
+/*
+ * Return 1 if there is no push_cert or if the push options in push_cert are
+ * the same as those in the argument; 0 otherwise.
+ */
+static int check_cert_push_options(const struct string_list *push_options)
+{
+	const char *buf = push_cert.buf;
+	int len = push_cert.len;
+
+	char *option;
+	const char *next_line;
+	int options_seen = 0;
+
+	int retval = 1;
+
+	if (!len)
+		return 1;
+
+	while ((option = find_header(buf, len, "push-option", &next_line))) {
+		len -= (next_line - buf);
+		buf = next_line;
+		options_seen++;
+		if (options_seen > push_options->nr
+		    || strcmp(option,
+			      push_options->items[options_seen - 1].string)) {
+			retval = 0;
+			goto leave;
+		}
+		free(option);
+	}
+
+	if (options_seen != push_options->nr)
+		retval = 0;
+
+leave:
+	free(option);
+	return retval;
+}
+
 static void prepare_push_cert_sha1(struct child_process *proc)
 {
 	static int already_done;
@@ -1929,6 +1971,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 		if (use_push_options)
 			read_push_options(&push_options);
+		if (!check_cert_push_options(&push_options)) {
+			struct command *cmd;
+			for (cmd = commands; cmd; cmd = cmd->next)
+				cmd->error_string = "inconsistent push options";
+		}
 
 		prepare_shallow_info(&si, &shallow);
 		if (!si.nr_ours && !si.nr_theirs)
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d446a5..5bcb288f5c 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '
 	test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPG 'inconsistent push options in signed push not allowed' '
+	# First, invoke receive-pack with dummy input to obtain its preamble.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	printf xxxx | test_might_fail git receive-pack dst >preamble &&
+
+	# Then, invoke push. Simulate a receive-pack that sends the preamble we
+	# obtained, followed by a dummy packet.
+	write_script myscript <<-\EOF &&
+		cat preamble &&
+		printf xxxx &&
+		cat >push
+	EOF
+	test_might_fail git push --push-option="foo" --push-option="bar" \
+		--receive-pack="\"$(pwd)/myscript\"" --signed dst --delete ff &&
+
+	# Replay the push output on a fresh dst, checking that ff is truly
+	# deleted.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push &&
+	test_must_fail git -C dst rev-parse ff &&
+
+	# Tweak the push output to make the push option outside the cert
+	# different, then replay it on a fresh dst, checking that ff is not
+	# deleted.
+	perl -pe "s/([^ ])bar/\$1baz/" push >push.tweak &&
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push.tweak >out &&
+	git -C dst rev-parse ff &&
+	grep "inconsistent push options" out
+'
+
 test_expect_success GPG 'fail without key and heed user.signingkey' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&