"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix gotd failing to protect refs
To:
gameoftrees@openbsd.org
Date:
Mon, 27 Jan 2025 00:13:00 +0100

Download raw body.

Thread
martijn@ pointed out on IRC that it is possible to modify a
protected branch via send -f, which is unexpected behaviour.

The reason for this is that gotd's reference protection checks only
happen if the client sends a pack file. In martijn's simple test case
the client doesn't send a pack file, and protection checks are skipped.

Our test suite didn't catch this because in our test the client has
always sent a pack file. This is due to an unrelated bug in the pack
creation code, where a parent commit far down the line is added to
the pack file (as the only commit) while it should have been dropped.
This looks like an error in got_pack_paint_commits().
I will investigate this second issue later.
For now, shortening the commit history used by our ref protection test
works around the pack creation bug and allows our test to cover the
problem martijn@ found.

ok?

M  gotd/repo_write.c                     |  53+  0-
M  regress/gotd/repo_write_protected.sh  |   1+  1-

2 files changed, 54 insertions(+), 1 deletion(-)

commit - 78399fc0398fa84bda2ffc8df65b6465db1d7500
commit + 3c7be384c7d1a2ab5a76b14cdf2eeb03df38fda0
blob - 12d6d0ad06ab75d791a22ba785294f8321da89df
blob + 1a330d17a4953ab17612922f651ca2ebfae95496
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -1505,6 +1505,51 @@ protect_refs_from_deletion(void)
 }
 
 static const struct got_error *
+protect_refs_from_moving(void)
+{
+	const struct got_error *err = NULL;
+	struct repo_write_client *client = &repo_write_client;
+	struct gotd_ref_update *ref_update;
+	struct got_pathlist_entry *pe;
+	const char *refname;
+
+	STAILQ_FOREACH(ref_update, &client->ref_updates, entry) {
+		if (ref_update->delete_ref)
+			continue;
+
+		if (got_object_id_cmp(&ref_update->old_id,
+		    &ref_update->new_id) == 0)
+			continue;
+
+		refname = got_ref_get_name(ref_update->ref);
+
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_tag_namespaces) {
+			err = protect_ref_namespace(refname, pe->path);
+			if (err)
+				return err;
+		}
+
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_branch_namespaces) {
+			err = protect_ref_namespace(refname, pe->path);
+			if (err)
+				return err;
+		}
+
+		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branches)
+		{
+			if (strcmp(refname, pe->path) == 0) {
+				return got_error_fmt(GOT_ERR_REF_PROTECTED,
+				    "%s", refname);
+			}
+		}
+	}
+
+	return NULL;
+}
+
+static const struct got_error *
 install_packfile(struct gotd_imsgev *iev)
 {
 	struct repo_write_client *client = &repo_write_client;
@@ -2251,6 +2296,14 @@ repo_write_dispatch_session(int fd, short event, void 
 				 */
 				repo_write.repo->pack_path_mtime.tv_sec = 0;
 				repo_write.repo->pack_path_mtime.tv_nsec = 0;
+			} else {
+				/*
+				 * Clients sending empty pack files might be
+				 * attempting to move a protected reference.
+				 */
+				err = protect_refs_from_moving();
+				if (err)
+					break;
 			}
 			err = update_refs(iev);
 			if (err) {
blob - ea1abadcdca2ecec752031ec65bd64a5796ddc1e
blob + d1e70129d2724139bccbc574d043f24c9ec5c7cd
--- regress/gotd/repo_write_protected.sh
+++ regress/gotd/repo_write_protected.sh
@@ -199,7 +199,7 @@ test_modify_protected_branch() {
 
 	got checkout $testroot/repo-clone $testroot/wt >/dev/null
 
-	for i in 1 2 3; do
+	for i in 1 2; do
 		echo "more alpha" >> $testroot/wt/alpha
 		(cd $testroot/wt && got commit -m "more" >/dev/null)
 	done