Keep the query string across the about redirects

The hop to the trailing-slash form of the about page, and the hop back
to the summary of a repository without a readme, were built from the
path alone, so a request for the about page of another branch landed
on the default one. The query goes into the Location line as the
client sent it, with any byte a header cannot carry percent-encoded.

6 files changed, 68 insertions, 12 deletions

diff --git a/source/cmd.c b/source/cmd.cindex 38fe596..49c1eb4 100644--- a/source/cmd.c+++ b/source/cmd.c@@ -34,7 +34,7 @@ static void head_fn(void)  static void about_fn(void) {-	char *currenturl, *redirect;+	char *currenturl, *query, *redirect; 	size_t path_info_len;  	if (!ctx.repo) {@@ -43,23 +43,28 @@ static void about_fn(void) 	}  	// The about page resolves relative links against its own URL, so it-	// only works with a trailing slash.+	// only works with a trailing slash. The branch and the rest of the+	// query travel along with either redirect. 	path_info_len = ctx.env.path_info ? strlen(ctx.env.path_info) : 0; 	if (!ctx.qry.path && 		(!ctx.qry.url || !*ctx.qry.url || ctx.qry.url[strlen(ctx.qry.url) - 1] != '/') && 		(!path_info_len || ctx.env.path_info[path_info_len - 1] != '/')) { 		currenturl = cgit_currenturl();+		query = cgit_currentquery(); 		redirect = cgit_fmtalloc("%s/", currenturl);-		cgit_redirect(redirect, true);+		cgit_redirect(redirect, query, true); 		free(currenturl);+		free(query); 		free(redirect); 	} else if (ctx.repo->readme.nr) { 		cgit_print_repo_readme(ctx.qry.path); 	} else { 		currenturl = cgit_currenturl();+		query = cgit_currentquery(); 		redirect = cgit_fmtalloc("%s../", currenturl);-		cgit_redirect(redirect, false);+		cgit_redirect(redirect, query, false); 		free(currenturl);+		free(query); 		free(redirect); 	} }
diff --git a/source/cmd.c b/source/cmd.c
index 38fe596..49c1eb4 100644
--- a/source/cmd.c
+++ b/source/cmd.c
@@ -34,7 +34,7 @@ static void head_fn(void)
 
 static void about_fn(void)
 {
-	char *currenturl, *redirect;
+	char *currenturl, *query, *redirect;
 	size_t path_info_len;
 
 	if (!ctx.repo) {
@@ -43,23 +43,28 @@ static void about_fn(void)
 	}
 
 	// The about page resolves relative links against its own URL, so it
-	// only works with a trailing slash.
+	// only works with a trailing slash. The branch and the rest of the
+	// query travel along with either redirect.
 	path_info_len = ctx.env.path_info ? strlen(ctx.env.path_info) : 0;
 	if (!ctx.qry.path &&
 		(!ctx.qry.url || !*ctx.qry.url || ctx.qry.url[strlen(ctx.qry.url) - 1] != '/') &&
 		(!path_info_len || ctx.env.path_info[path_info_len - 1] != '/')) {
 		currenturl = cgit_currenturl();
+		query = cgit_currentquery();
 		redirect = cgit_fmtalloc("%s/", currenturl);
-		cgit_redirect(redirect, true);
+		cgit_redirect(redirect, query, true);
 		free(currenturl);
+		free(query);
 		free(redirect);
 	} else if (ctx.repo->readme.nr) {
 		cgit_print_repo_readme(ctx.qry.path);
 	} else {
 		currenturl = cgit_currenturl();
+		query = cgit_currentquery();
 		redirect = cgit_fmtalloc("%s../", currenturl);
-		cgit_redirect(redirect, false);
+		cgit_redirect(redirect, query, false);
 		free(currenturl);
+		free(query);
 		free(redirect);
 	}
 }
diff --git a/source/html.c b/source/html.cindex a1d244b..96b1b52 100644--- a/source/html.c+++ b/source/html.c@@ -339,6 +339,28 @@ void html_header_arg_in_quotes(const char *txt) 		html(txt); } +/*+ * A query string goes into a header the way it arrived, already encoded by+ * the client, except that a byte a header line cannot carry is percent-encoded+ * so the line stays one line.+ */+void html_header_query(const char *txt)+{+	const char *p = txt;++	while (p && *p) {+		unsigned char c = *p;+		if (c <= ' ' || c >= 0x7f) {+			html_raw(txt, p - txt);+			html(url_escape_table[c]);+			txt = p + 1;+		}+		p++;+	}+	if (p != txt)+		html(txt);+}+ void html_hidden(const char *name, const char *value) { 	html("<input type='hidden' name='");
diff --git a/source/html.c b/source/html.c
index a1d244b..96b1b52 100644
--- a/source/html.c
+++ b/source/html.c
@@ -339,6 +339,28 @@ void html_header_arg_in_quotes(const char *txt)
 		html(txt);
 }
 
+/*
+ * A query string goes into a header the way it arrived, already encoded by
+ * the client, except that a byte a header line cannot carry is percent-encoded
+ * so the line stays one line.
+ */
+void html_header_query(const char *txt)
+{
+	const char *p = txt;
+
+	while (p && *p) {
+		unsigned char c = *p;
+		if (c <= ' ' || c >= 0x7f) {
+			html_raw(txt, p - txt);
+			html(url_escape_table[c]);
+			txt = p + 1;
+		}
+		p++;
+	}
+	if (p != txt)
+		html(txt);
+}
+
 void html_hidden(const char *name, const char *value)
 {
 	html("<input type='hidden' name='");
diff --git a/source/html.h b/source/html.hindex 46b445f..56fcb7f 100644--- a/source/html.h+++ b/source/html.h@@ -63,6 +63,7 @@ extern void html_attr(const char *txt); extern void html_url_path(const char *txt); extern void html_url_arg(const char *txt); extern void html_header_arg_in_quotes(const char *txt);+extern void html_header_query(const char *txt); extern void html_hidden(const char *name, const char *value);  __attribute__((format (printf,1,2)))
diff --git a/source/html.h b/source/html.h
index 46b445f..56fcb7f 100644
--- a/source/html.h
+++ b/source/html.h
@@ -63,6 +63,7 @@ extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
 extern void html_url_arg(const char *txt);
 extern void html_header_arg_in_quotes(const char *txt);
+extern void html_header_query(const char *txt);
 extern void html_hidden(const char *name, const char *value);
 
 __attribute__((format (printf,1,2)))
diff --git a/source/ui-shared.c b/source/ui-shared.cindex 0f2f1e3..147472e 100644--- a/source/ui-shared.c+++ b/source/ui-shared.c@@ -828,16 +828,15 @@ char *cgit_currenturl(void) 	return cgit_fmtalloc("%s/%s", root, ctx.qry.url); } -char *cgit_currentfullurl(void)+char *cgit_currentquery(void) {-	const char *root = root_url(); 	const char *orig_query = ctx.env.query_string ? ctx.env.query_string : ""; 	size_t len = strlen(orig_query); 	char *query = xmalloc(len + 2);-	char *match, *ret;+	char *match; -	// The url argument carried the path into this request and the path is-	// spelled out again below, so it is dropped here. The copy keeps a+	// The url argument carried the path into this request and every caller+	// spells the path out again, so it is dropped here. The copy keeps a 	// leading question mark so that every match has a character before it, 	// which is what tells the argument itself from the tail of another one. 	memcpy(query + 1, orig_query, len + 1);@@ -856,6 +855,14 @@ char *cgit_currentfullurl(void) 	} 	if (!query[1]) 		query[0] = '\0';+	return query;+}++char *cgit_currentfullurl(void)+{+	const char *root = root_url();+	char *query = cgit_currentquery();+	char *ret;  	if (!ctx.qry.url) 		ret = cgit_fmtalloc("%s%s", root, query);@@ -1453,11 +1460,13 @@ void cgit_print_http_headers(void) 		exit(0); } -void cgit_redirect(const char *url, bool permanent)+void cgit_redirect(const char *url, const char *query, bool permanent) { 	htmlf("Status: %d %s\n", permanent ? 301 : 302, permanent ? "Moved Permanently" : "Found"); 	html("Location: "); 	html_url_path(url);+	if (query)+		html_header_query(query); 	html("\n\n"); } 
diff --git a/source/ui-shared.c b/source/ui-shared.c
index 0f2f1e3..147472e 100644
--- a/source/ui-shared.c
+++ b/source/ui-shared.c
@@ -828,16 +828,15 @@ char *cgit_currenturl(void)
 	return cgit_fmtalloc("%s/%s", root, ctx.qry.url);
 }
 
-char *cgit_currentfullurl(void)
+char *cgit_currentquery(void)
 {
-	const char *root = root_url();
 	const char *orig_query = ctx.env.query_string ? ctx.env.query_string : "";
 	size_t len = strlen(orig_query);
 	char *query = xmalloc(len + 2);
-	char *match, *ret;
+	char *match;
 
-	// The url argument carried the path into this request and the path is
-	// spelled out again below, so it is dropped here. The copy keeps a
+	// The url argument carried the path into this request and every caller
+	// spells the path out again, so it is dropped here. The copy keeps a
 	// leading question mark so that every match has a character before it,
 	// which is what tells the argument itself from the tail of another one.
 	memcpy(query + 1, orig_query, len + 1);
@@ -856,6 +855,14 @@ char *cgit_currentfullurl(void)
 	}
 	if (!query[1])
 		query[0] = '\0';
+	return query;
+}
+
+char *cgit_currentfullurl(void)
+{
+	const char *root = root_url();
+	char *query = cgit_currentquery();
+	char *ret;
 
 	if (!ctx.qry.url)
 		ret = cgit_fmtalloc("%s%s", root, query);
@@ -1453,11 +1460,13 @@ void cgit_print_http_headers(void)
 		exit(0);
 }
 
-void cgit_redirect(const char *url, bool permanent)
+void cgit_redirect(const char *url, const char *query, bool permanent)
 {
 	htmlf("Status: %d %s\n", permanent ? 301 : 302, permanent ? "Moved Permanently" : "Found");
 	html("Location: ");
 	html_url_path(url);
+	if (query)
+		html_header_query(query);
 	html("\n\n");
 }
 
diff --git a/source/ui-shared.h b/source/ui-shared.hindex 4809bc1..c4b96a9 100644--- a/source/ui-shared.h+++ b/source/ui-shared.h@@ -18,10 +18,12 @@ extern char *cgit_hosturl(void); /*  * The URL of the request being answered, which cgit_currentfullurl gives with  * the query string on the end as well, minus the url argument that the path- * was taken from.+ * was taken from. cgit_currentquery is that query string alone, led by its+ * question mark, or an empty string when nothing is left of it.  */ extern char *cgit_currenturl(void); extern char *cgit_currentfullurl(void);+extern char *cgit_currentquery(void); extern const char *cgit_loginurl(void); extern char *cgit_repourl(const char *reponame); extern void cgit_print_clone_row(const char *url, int colspan);@@ -85,7 +87,11 @@ extern struct date_mode cgit_date_mode(enum date_mode_type type);  */ extern void cgit_print_age(time_t t, int tz, time_t max_relative); extern void cgit_print_http_headers(void);-extern void cgit_redirect(const char *url, bool permanent);+/*+ * Answer with a redirect to url, followed by query when one is given, which+ * is written as it arrived.+ */+extern void cgit_redirect(const char *url, const char *query, bool permanent); extern void cgit_print_docstart(void); extern void cgit_print_docend(void); __attribute__((format (printf,3,4)))
diff --git a/source/ui-shared.h b/source/ui-shared.h
index 4809bc1..c4b96a9 100644
--- a/source/ui-shared.h
+++ b/source/ui-shared.h
@@ -18,10 +18,12 @@ extern char *cgit_hosturl(void);
 /*
  * The URL of the request being answered, which cgit_currentfullurl gives with
  * the query string on the end as well, minus the url argument that the path
- * was taken from.
+ * was taken from. cgit_currentquery is that query string alone, led by its
+ * question mark, or an empty string when nothing is left of it.
  */
 extern char *cgit_currenturl(void);
 extern char *cgit_currentfullurl(void);
+extern char *cgit_currentquery(void);
 extern const char *cgit_loginurl(void);
 extern char *cgit_repourl(const char *reponame);
 extern void cgit_print_clone_row(const char *url, int colspan);
@@ -85,7 +87,11 @@ extern struct date_mode cgit_date_mode(enum date_mode_type type);
  */
 extern void cgit_print_age(time_t t, int tz, time_t max_relative);
 extern void cgit_print_http_headers(void);
-extern void cgit_redirect(const char *url, bool permanent);
+/*
+ * Answer with a redirect to url, followed by query when one is given, which
+ * is written as it arrived.
+ */
+extern void cgit_redirect(const char *url, const char *query, bool permanent);
 extern void cgit_print_docstart(void);
 extern void cgit_print_docend(void);
 __attribute__((format (printf,3,4)))
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.shindex be98644..73c2ca5 100755--- a/tests/t0303-robustness.sh+++ b/tests/t0303-robustness.sh@@ -507,6 +507,19 @@ test_expect_success 'the dumb transport withholds the alternates file' ' 	grep "^Status: 200" tmp ' +# The about page redirects to its trailing-slash form so relative links+# resolve, and the branch asked for has to survive that hop, as does the+# hop back to the summary of a repository without a readme.+test_expect_success 'the about redirects keep the query string' '+	git -C repos/rob branch other HEAD &&+	CGIT_CONFIG="$PWD/aboutrc" QUERY_STRING="url=rob/about&h=other&x=1" cgit >tmp &&+	grep "^Status: 301" tmp &&+	grep "^Location: /rob/about/?h=other&x=1$" tmp &&+	robq "url=rob/about/&h=other" >tmp &&+	grep "^Status: 302" tmp &&+	grep "^Location: /rob/about/../?h=other$" tmp+'+ test_expect_success 'a symlink whose target is a large blob is listed without it' ' 	big=$(head -c 5000 /dev/zero | tr "\0" a | git -C repos/rob hash-object -w --stdin) && 	(
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.sh
index be98644..73c2ca5 100755
--- a/tests/t0303-robustness.sh
+++ b/tests/t0303-robustness.sh
@@ -507,6 +507,19 @@ test_expect_success 'the dumb transport withholds the alternates file' '
 	grep "^Status: 200" tmp
 '
 
+# The about page redirects to its trailing-slash form so relative links
+# resolve, and the branch asked for has to survive that hop, as does the
+# hop back to the summary of a repository without a readme.
+test_expect_success 'the about redirects keep the query string' '
+	git -C repos/rob branch other HEAD &&
+	CGIT_CONFIG="$PWD/aboutrc" QUERY_STRING="url=rob/about&h=other&x=1" cgit >tmp &&
+	grep "^Status: 301" tmp &&
+	grep "^Location: /rob/about/?h=other&x=1$" tmp &&
+	robq "url=rob/about/&h=other" >tmp &&
+	grep "^Status: 302" tmp &&
+	grep "^Location: /rob/about/../?h=other$" tmp
+'
+
 test_expect_success 'a symlink whose target is a large blob is listed without it' '
 	big=$(head -c 5000 /dev/zero | tr "\0" a | git -C repos/rob hash-object -w --stdin) &&
 	(