5 files changed, 71 insertions, 1 deletion

diff --git a/source/cache.c b/source/cache.cindex e916728..c6d0427 100644--- a/source/cache.c+++ b/source/cache.c@@ -75,6 +75,15 @@ static int open_slot(struct cache_slot *slot) 	return 0; } +/* A key longer than the buffer above can never be read back, so a slot keyed+ * on one would never match and every such request would regenerate its page+ * while still writing a slot nothing can use. Those requests skip the cache+ * instead. */+static int key_fits_slot(const char *key)+{+	return strlen(key) + 1 <= CACHE_BUFSIZE;+}+ /* Close the active cache slot */ static int close_slot(struct cache_slot *slot) {@@ -379,6 +388,12 @@ int cache_process(int size, const char *path, const char *key, int ttl, 	} 	if (!key) 		key = "";+	if (!key_fits_slot(key)) {+		cache_log("[cgit] Cache key too long for a slot, caching is "+			  "disabled for this request\n");+		fn();+		return 0;+	} 	hash = cache_hash_str(key) % size; 	strbuf_addstr(&filename, path); 	strbuf_ensure_end(&filename, '/');
diff --git a/source/cache.c b/source/cache.c
index e916728..c6d0427 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -75,6 +75,15 @@ static int open_slot(struct cache_slot *slot)
 	return 0;
 }
 
+/* A key longer than the buffer above can never be read back, so a slot keyed
+ * on one would never match and every such request would regenerate its page
+ * while still writing a slot nothing can use. Those requests skip the cache
+ * instead. */
+static int key_fits_slot(const char *key)
+{
+	return strlen(key) + 1 <= CACHE_BUFSIZE;
+}
+
 /* Close the active cache slot */
 static int close_slot(struct cache_slot *slot)
 {
@@ -379,6 +388,12 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	}
 	if (!key)
 		key = "";
+	if (!key_fits_slot(key)) {
+		cache_log("[cgit] Cache key too long for a slot, caching is "
+			  "disabled for this request\n");
+		fn();
+		return 0;
+	}
 	hash = cache_hash_str(key) % size;
 	strbuf_addstr(&filename, path);
 	strbuf_ensure_end(&filename, '/');
diff --git a/source/cgit.c b/source/cgit.cindex 2ecfb4f..7cce3d3 100644--- a/source/cgit.c+++ b/source/cgit.c@@ -360,7 +360,14 @@ static void querystring_cb(const char *name, const char *value) 	} else if (!strcmp(name, "qt")) { 		ctx.qry.grep = xstrdup(value); 	} else if (!strcmp(name, "q")) {-		ctx.qry.search = xstrdup(value);+		/* A query is matched against every repository, ref or commit+		 * the page lists, so bound what one request can ask to be+		 * compared. Nothing legible reaches this length, and the value+		 * also lands in the cache key. */+		if (strlen(value) > CGIT_MAX_SEARCH_LEN)+			ctx.qry.search = xstrndup(value, CGIT_MAX_SEARCH_LEN);+		else+			ctx.qry.search = xstrdup(value); 	} else if (!strcmp(name, "h")) { 		ctx.qry.head = xstrdup(value); 		ctx.qry.has_symref = 1;
diff --git a/source/cgit.c b/source/cgit.c
index 2ecfb4f..7cce3d3 100644
--- a/source/cgit.c
+++ b/source/cgit.c
@@ -360,7 +360,14 @@ static void querystring_cb(const char *name, const char *value)
 	} else if (!strcmp(name, "qt")) {
 		ctx.qry.grep = xstrdup(value);
 	} else if (!strcmp(name, "q")) {
-		ctx.qry.search = xstrdup(value);
+		/* A query is matched against every repository, ref or commit
+		 * the page lists, so bound what one request can ask to be
+		 * compared. Nothing legible reaches this length, and the value
+		 * also lands in the cache key. */
+		if (strlen(value) > CGIT_MAX_SEARCH_LEN)
+			ctx.qry.search = xstrndup(value, CGIT_MAX_SEARCH_LEN);
+		else
+			ctx.qry.search = xstrdup(value);
 	} else if (!strcmp(name, "h")) {
 		ctx.qry.head = xstrdup(value);
 		ctx.qry.has_symref = 1;
diff --git a/source/cgit.h b/source/cgit.hindex 8f10068..b05876e 100644--- a/source/cgit.h+++ b/source/cgit.h@@ -55,6 +55,14 @@  #define BIT(x)	(1U << (x)) +/*+ * Longest search string a request may supply. The filter bar matches its+ * query against every item a page lists, so this bounds the work one request+ * can ask for. It is not a configuration knob, in the same way as the ofs+ * ceiling in querystring_cb.+ */+#define CGIT_MAX_SEARCH_LEN 512+ typedef void (*configfn)(const char *name, const char *value); typedef void (*filepair_fn)(struct diff_filepair *pair); typedef void (*linediff_fn)(char *line, int len);
diff --git a/source/cgit.h b/source/cgit.h
index 8f10068..b05876e 100644
--- a/source/cgit.h
+++ b/source/cgit.h
@@ -55,6 +55,14 @@
 
 #define BIT(x)	(1U << (x))
 
+/*
+ * Longest search string a request may supply. The filter bar matches its
+ * query against every item a page lists, so this bounds the work one request
+ * can ask for. It is not a configuration knob, in the same way as the ofs
+ * ceiling in querystring_cb.
+ */
+#define CGIT_MAX_SEARCH_LEN 512
+
 typedef void (*configfn)(const char *name, const char *value);
 typedef void (*filepair_fn)(struct diff_filepair *pair);
 typedef void (*linediff_fn)(char *line, int len);
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.shindex 7e6334c..510e51c 100755--- a/tests/t0020-validate-cache.sh+++ b/tests/t0020-validate-cache.sh@@ -124,4 +124,25 @@ test_expect_success 'the page ends where it should, so nothing was dropped' ' 	tail -c 200 big.second.body | grep "</html>" ' +# --- A key too long to read back must not claim a slot ----------------------+# A slot stores its key ahead of the content and only the first few kilobytes+# are read back, so a longer key could never match. Such a request used to+# write a slot on every visit that no later request could ever use.+test_expect_success 'an over-long cache key leaves no slot behind' '+	rm -rf cache3 && mkdir cache3 &&+	sed -e "s|^cache-root=.*|cache-root=$PWD/cache3|" bigrc >bigkeyrc &&+	long=$(awk "BEGIN{s=\"\";for(i=0;i<6000;i++)s=s \"k\"; print s}") &&+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/&q=$long" cgit >key.out 2>key.err &&+	grep "</html>" key.out &&+	ls cache3 >key.slots &&+	test_line_count = 0 key.slots+'++test_expect_success 'an ordinary key still fills a slot' '+	rm -rf cache3 && mkdir cache3 &&+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >/dev/null &&+	ls cache3 >key.slots &&+	test_line_count = 1 key.slots+'+ test_done
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.sh
index 7e6334c..510e51c 100755
--- a/tests/t0020-validate-cache.sh
+++ b/tests/t0020-validate-cache.sh
@@ -124,4 +124,25 @@ test_expect_success 'the page ends where it should, so nothing was dropped' '
 	tail -c 200 big.second.body | grep "</html>"
 '
 
+# --- A key too long to read back must not claim a slot ----------------------
+# A slot stores its key ahead of the content and only the first few kilobytes
+# are read back, so a longer key could never match. Such a request used to
+# write a slot on every visit that no later request could ever use.
+test_expect_success 'an over-long cache key leaves no slot behind' '
+	rm -rf cache3 && mkdir cache3 &&
+	sed -e "s|^cache-root=.*|cache-root=$PWD/cache3|" bigrc >bigkeyrc &&
+	long=$(awk "BEGIN{s=\"\";for(i=0;i<6000;i++)s=s \"k\"; print s}") &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/&q=$long" cgit >key.out 2>key.err &&
+	grep "</html>" key.out &&
+	ls cache3 >key.slots &&
+	test_line_count = 0 key.slots
+'
+
+test_expect_success 'an ordinary key still fills a slot' '
+	rm -rf cache3 && mkdir cache3 &&
+	CGIT_CONFIG="$PWD/bigkeyrc" QUERY_STRING="url=bigpage/" cgit >/dev/null &&
+	ls cache3 >key.slots &&
+	test_line_count = 1 key.slots
+'
+
 test_done
diff --git a/tests/t0201-limits.sh b/tests/t0201-limits.shindex a8dce62..72d8a23 100755--- a/tests/t0201-limits.sh+++ b/tests/t0201-limits.sh@@ -43,6 +43,25 @@ test_expect_success 'set up limit fixtures' '  limitq() { CGIT_CONFIG="$PWD/limitrc" QUERY_STRING="$1" cgit; } +# --- A request cannot ask for an unbounded amount of matching ---------------+# The query is compared against every repository the index lists, so an+# enormous one would multiply out across the whole listing. It is clamped+# rather than rejected, so an ordinary query still narrows the page.+test_expect_success 'an enormous query is clamped, not rejected' '+	long=$(awk "BEGIN{s=\"\";for(i=0;i<4000;i++)s=s \"a\"; print s}") &&+	test ${#long} -eq 4000 &&+	cgit_query "q=$long" >tmp &&+	grep "</html>" tmp &&+	longest=$(grep -o "aaaa*" tmp | awk "{print length}" | sort -n | tail -1) &&+	test "$longest" -eq 512+'++test_expect_success 'an ordinary query still filters the index' '+	cgit_query "q=foo" >tmp &&+	grep "foo" tmp &&+	! grep ">bar<" tmp+'+ # --- The refs page caps each section and links to the category pages -------- test_expect_success 'refs page lists max-ref-count branches and tags' ' 	limitq "url=limits/refs/" >tmp &&
diff --git a/tests/t0201-limits.sh b/tests/t0201-limits.sh
index a8dce62..72d8a23 100755
--- a/tests/t0201-limits.sh
+++ b/tests/t0201-limits.sh
@@ -43,6 +43,25 @@ test_expect_success 'set up limit fixtures' '
 
 limitq() { CGIT_CONFIG="$PWD/limitrc" QUERY_STRING="$1" cgit; }
 
+# --- A request cannot ask for an unbounded amount of matching ---------------
+# The query is compared against every repository the index lists, so an
+# enormous one would multiply out across the whole listing. It is clamped
+# rather than rejected, so an ordinary query still narrows the page.
+test_expect_success 'an enormous query is clamped, not rejected' '
+	long=$(awk "BEGIN{s=\"\";for(i=0;i<4000;i++)s=s \"a\"; print s}") &&
+	test ${#long} -eq 4000 &&
+	cgit_query "q=$long" >tmp &&
+	grep "</html>" tmp &&
+	longest=$(grep -o "aaaa*" tmp | awk "{print length}" | sort -n | tail -1) &&
+	test "$longest" -eq 512
+'
+
+test_expect_success 'an ordinary query still filters the index' '
+	cgit_query "q=foo" >tmp &&
+	grep "foo" tmp &&
+	! grep ">bar<" tmp
+'
+
 # --- The refs page caps each section and links to the category pages --------
 test_expect_success 'refs page lists max-ref-count branches and tags' '
 	limitq "url=limits/refs/" >tmp &&