From 3ea188e2bb5e7994645a03e19bf613369bc20585 Mon Sep 17 00:00:00 2001
From: Bryce Kwon <bryce@brycekwon.com>
Date: Sat, 8 Aug 2026 12:49:45 -1000
Subject: Expand `module-link` placeholders outside printf

---
 source/ui-shared.c      | 42 +++++++++++++++++++++++++++--
 tests/t0200-security.sh | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/source/ui-shared.c b/source/ui-shared.c
index 1b65a7f..125a235 100644
--- a/source/ui-shared.c
+++ b/source/ui-shared.c
@@ -616,6 +616,39 @@ static struct string_list_item *lookup_path(struct string_list *list,
 	return NULL;
 }
 
+/*
+ * Expand the %s placeholders in a module-link template and emit the result as
+ * an attribute value. The template is never handed to printf: a repository can
+ * supply its own through a repo-local cgitrc, and a surplus conversion would
+ * then read past the argument list.
+ */
+static void html_module_link(const char *tmpl, const char **args, int nargs)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int used = 0;
+
+	while (*tmpl) {
+		if (*tmpl != '%') {
+			strbuf_addch(&sb, *tmpl++);
+			continue;
+		}
+		tmpl++;
+		if (*tmpl == '%') {
+			strbuf_addch(&sb, '%');
+			tmpl++;
+		} else if (*tmpl == 's' && used < nargs) {
+			strbuf_addstr(&sb, args[used++]);
+			tmpl++;
+		} else {
+			// Anything else is not a placeholder this template can
+			// fill, so keep it verbatim rather than dropping it.
+			strbuf_addch(&sb, '%');
+		}
+	}
+	html_attr(sb.buf);
+	strbuf_release(&sb);
+}
+
 void cgit_submodule_link(const char *class, char *path, const char *rev)
 {
 	struct string_list *list;
@@ -641,14 +674,19 @@ void cgit_submodule_link(const char *class, char *path, const char *rev)
 			htmlf("class='%s' ", class);
 		html("href='");
 		if (item) {
-			html_attrf(item->util, rev);
+			// A per-path template names only the submodule commit.
+			const char *args[] = { rev };
+			html_module_link(item->util, args, 1);
 		} else {
+			const char *args[2];
 			dir = strrchr(path, '/');
 			if (dir)
 				dir++;
 			else
 				dir = path;
-			html_attrf(ctx.repo->module_link, dir, rev);
+			args[0] = dir;
+			args[1] = rev;
+			html_module_link(ctx.repo->module_link, args, 2);
 		}
 		html("'>");
 		html_txt(path);
diff --git a/tests/t0200-security.sh b/tests/t0200-security.sh
index de248ca..df60174 100755
--- a/tests/t0200-security.sh
+++ b/tests/t0200-security.sh
@@ -172,4 +172,75 @@ test_expect_success 'a message-less commit renders without crashing' '
 	grep "no commit message" tmp
 '
 
+# --- A repository cannot supply a printf format string ----------------------
+# module-link is a template, and scan-path lets a repository set it through its
+# own cgitrc. Handing it to printf let a repo owner crash the process, or read
+# stack memory into the served page, with surplus conversions.
+test_expect_success 'set up a submodule fixture with a hostile module-link' '
+	mkrepo repos/modlink 1 &&
+	(
+		cd repos/modlink &&
+		sub=$(git rev-parse HEAD) &&
+		git update-index --add --cacheinfo 160000,$sub,submod &&
+		git commit -m gitlink
+	) &&
+	mkdir -p scan &&
+	cp -R repos/modlink scan/modlink &&
+	printf "module-link=/m/%%s/%%s/%%s/%%s/%%s/%%s/%%s/%%s/%%s/%%s\n" \
+		>scan/modlink/.git/cgitrc &&
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "scan-path=$PWD/scan"
+	} >modlinkrc
+'
+
+test_expect_success 'a surplus conversion does not crash the tree view' '
+	CGIT_CONFIG="$PWD/modlinkrc" QUERY_STRING="url=modlink/tree/" cgit >tmp &&
+	grep "ls-mod" tmp
+'
+
+test_expect_success 'a surplus conversion is shown literally, not filled' '
+	grep "href=./m/submod/[0-9a-f]*/%s/%s/" tmp
+'
+
+test_expect_success 'a well-formed module-link still takes path and sha1' '
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "module-link=/mod/%s/commit/?id=%s" &&
+		echo "repo.url=modlink" &&
+		echo "repo.path=$PWD/repos/modlink/.git"
+	} >modlinkokrc &&
+	sub=$(git -C repos/modlink rev-parse HEAD~1) &&
+	CGIT_CONFIG="$PWD/modlinkokrc" QUERY_STRING="url=modlink/tree/" cgit >tmp &&
+	grep "href=./mod/submod/commit/?id=$sub." tmp
+'
+
+test_expect_success 'a per-path module-link takes only the sha1' '
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "repo.url=modlink" &&
+		echo "repo.path=$PWD/repos/modlink/.git" &&
+		echo "repo.module-link.submod=https://example.com/s/?id=%s"
+	} >modlinkpathrc &&
+	sub=$(git -C repos/modlink rev-parse HEAD~1) &&
+	CGIT_CONFIG="$PWD/modlinkpathrc" QUERY_STRING="url=modlink/tree/" cgit >tmp &&
+	grep "href=.https://example.com/s/?id=$sub." tmp
+'
+
+test_expect_success 'a doubled percent in a module-link renders as one' '
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "module-link=/m/%s/%%/%s" &&
+		echo "repo.url=modlink" &&
+		echo "repo.path=$PWD/repos/modlink/.git"
+	} >modlinkpctrc &&
+	sub=$(git -C repos/modlink rev-parse HEAD~1) &&
+	CGIT_CONFIG="$PWD/modlinkpctrc" QUERY_STRING="url=modlink/tree/" cgit >tmp &&
+	grep "href=./m/submod/%/$sub." tmp
+'
+
 test_done
-- 
cgit v2.8.0