Restyle the sources and fix the audit's findings

1 file changed, 71 insertions, 36 deletions

diff --git a/source/ui-clone.c b/source/ui-clone.c
index 71bb2ed..70e4446 100644
--- a/source/ui-clone.c
+++ b/source/ui-clone.c
@@ -1,40 +1,64 @@
-/* ui-clone.c: functions for http cloning, based on
- * git's http-backend.c by Shawn O. Pearce
- *
- * Copyright (C) 2006-2014 cgit Development Team <cgit@lists.zx2c4.com>
- *
- * Licensed under GNU General Public License v2
- *   (see LICENSE.txt for full license text)
+/*
+ * The dumb HTTP transport, which lets a client clone by fetching plain files
+ * rather than by talking to a server side helper. It answers the requests
+ * such a client makes, the ref listing under info, the loose objects and pack
+ * files under objects, and HEAD, each written as raw bytes rather than as a
+ * page. The two listings a clone starts from are built per request, so a
+ * repository serves without anyone having run git update-server-info over it.
+ * The shape of it all follows git's own http-backend.
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "cgit.h"
-#include "ui-clone.h"
 #include "html.h"
+#include "ui-clone.h"
 #include "ui-shared.h"
-#include "packfile.h"
 
-static int print_ref_info(const struct reference *ref, void *cb_data)
+/*
+ * A client reading this listing expects an annotated tag to be followed by a
+ * second line, marked with ^{}, naming the object the tag peels to, the
+ * format git update-server-info writes into info/refs.
+ */
+static int print_ref(const struct reference *ref, void *cb_data)
 {
 	struct object *obj;
 
-	if (!(obj = parse_object(the_repository, ref->oid)))
+	obj = parse_object(the_repository, ref->oid);
+	if (!obj)
 		return 0;
 
 	htmlf("%s\t%s\n", oid_to_hex(ref->oid), ref->name);
 	if (obj->type == OBJ_TAG) {
-		if (!(obj = deref_tag(the_repository, obj, ref->name, 0)))
+		obj = deref_tag(the_repository, obj, ref->name, 0);
+		if (!obj)
 			return 0;
 		htmlf("%s\t%s^{}\n", oid_to_hex(&obj->oid), ref->name);
 	}
 	return 0;
 }
 
+/*
+ * A path ending in a slash is handed back whole, where git's own
+ * pack_basename would return the empty string.
+ */
+static const char *last_path_component(const char *path)
+{
+	const char *slash = strrchr(path, '/');
+
+	if (slash && slash[1] != '\0')
+		return slash + 1;
+	return path;
+}
+
+/*
+ * Only the packs this repository holds itself are listed, because one
+ * borrowed from an alternate is not reachable below this URL and a client
+ * told about it would come back for a file that is not there.
+ */
 static void print_pack_info(void)
 {
 	struct odb_source *source;
-	char *offset;
 
 	ctx.page.mimetype = "text/plain";
 	ctx.page.filename = "objects/info/packs";
@@ -42,21 +66,38 @@ static void print_pack_info(void)
 	odb_reprepare(the_repository->objects);
 	for (source = the_repository->objects->sources; source; source = source->next) {
 		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct packfile_list_entry *e;
-		for (e = files->packed->packs.head; e; e = e->next) {
-			struct packed_git *p = e->pack;
-			if (p->pack_local) {
-				offset = strrchr(p->pack_name, '/');
-				if (offset && offset[1] != '\0')
-					++offset;
-				else
-					offset = p->pack_name;
-				htmlf("P %s\n", offset);
-			}
+		struct packfile_list_entry *entry;
+		// Asked for through the accessor rather than read off the list,
+		// because a pack the multi-pack-index already covers joins that
+		// list only when the accessor loads it, and reading the field
+		// directly leaves those packs unfindable.
+		for (entry = packfile_store_get_packs(files->packed); entry;
+		     entry = entry->next) {
+			struct packed_git *pack = entry->pack;
+			if (pack->pack_local)
+				htmlf("P %s\n",
+				      last_path_component(pack->pack_name));
 		}
 	}
 }
 
+/*
+ * Beyond the obvious directory traversal, the strict character set heads off
+ * other funny business, for example the file name quirks of the Cygwin port.
+ */
+static int path_is_safe(const char *path)
+{
+	const char *p;
+
+	for (p = path; *p; ++p) {
+		if (*p == '.' && *(p + 1) == '.')
+			return 0;
+		if (!isalnum((unsigned char)*p) && *p != '/' && *p != '.' && *p != '-')
+			return 0;
+	}
+	return 1;
+}
+
 static void send_file(const char *path)
 {
 	struct stat st;
@@ -75,6 +116,8 @@ static void send_file(const char *path)
 		return;
 	}
 	ctx.page.mimetype = "application/octet-stream";
+	// Offer the file under its path inside the repository, so the layout
+	// of the server's disk stays out of the download name.
 	ctx.page.filename = path;
 	skip_prefix(path, ctx.repo->path, &ctx.page.filename);
 	skip_prefix(ctx.page.filename, "/", &ctx.page.filename);
@@ -93,12 +136,12 @@ void cgit_clone_info(void)
 	ctx.page.filename = "info/refs";
 	cgit_print_http_headers();
 	refs_for_each_ref(get_main_ref_store(the_repository),
-			  print_ref_info, NULL);
+			  print_ref, NULL);
 }
 
 void cgit_clone_objects(void)
 {
-	char *p, *path;
+	char *path;
 
 	if (!ctx.qry.path)
 		goto err;
@@ -108,16 +151,8 @@ void cgit_clone_objects(void)
 		return;
 	}
 
-	/* Avoid directory traversal by forbidding "..", but also work around
-	 * other funny business by just specifying a fairly strict format. For
-	 * example, now we don't have to stress out about the Cygwin port.
-	 */
-	for (p = ctx.qry.path; *p; ++p) {
-		if (*p == '.' && *(p + 1) == '.')
-			goto err;
-		if (!isalnum((unsigned char)*p) && *p != '/' && *p != '.' && *p != '-')
-			goto err;
-	}
+	if (!path_is_safe(ctx.qry.path))
+		goto err;
 
 	path = repo_git_path(the_repository, "objects/%s", ctx.qry.path);
 	send_file(path);