Encode repository urls and the feed branch

Links built from the repository url, among them the description link
on the index and the Atom and refs links, wrote it as configured, so a
name holding a space, '#' or '?' broke them. The feed link in the page
head did the same with the branch, which turned a '+' into a space.

6 files changed, 67 insertions, 27 deletions

diff --git a/source/html.c b/source/html.cindex 367cea7..1a6b7c5 100644--- a/source/html.c+++ b/source/html.c@@ -314,6 +314,18 @@ void html_url_path(const char *txt) 		html(txt); } +void cgit_add_url_path(struct strbuf *sb, const char *txt)+{+	for (; *txt; txt++) {+		const char *esc = url_escape_table[(unsigned char)*txt];++		if (esc)+			strbuf_addstr(sb, esc);+		else+			strbuf_addch(sb, *txt);+	}+}+ void html_url_arg(const char *txt) { 	const char *p = txt;
diff --git a/source/html.c b/source/html.c
index 367cea7..1a6b7c5 100644
--- a/source/html.c
+++ b/source/html.c
@@ -314,6 +314,18 @@ void html_url_path(const char *txt)
 		html(txt);
 }
 
+void cgit_add_url_path(struct strbuf *sb, const char *txt)
+{
+	for (; *txt; txt++) {
+		const char *esc = url_escape_table[(unsigned char)*txt];
+
+		if (esc)
+			strbuf_addstr(sb, esc);
+		else
+			strbuf_addch(sb, *txt);
+	}
+}
+
 void html_url_arg(const char *txt)
 {
 	const char *p = txt;
diff --git a/source/html.h b/source/html.hindex 1960b6a..095804c 100644--- a/source/html.h+++ b/source/html.h@@ -62,6 +62,14 @@ extern ssize_t html_ntxt(const char *txt, size_t len);  extern void html_attr(const char *txt); extern void html_url_path(const char *txt);++/*+ * Append txt to sb with the escaping html_url_path writes, for a url that is+ * assembled before it reaches a sink. Nothing is captured, so it is safe to+ * call while a capture is open.+ */+extern void cgit_add_url_path(struct strbuf *sb, 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);
diff --git a/source/html.h b/source/html.h
index 1960b6a..095804c 100644
--- a/source/html.h
+++ b/source/html.h
@@ -62,6 +62,14 @@ extern ssize_t html_ntxt(const char *txt, size_t len);
 
 extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
+
+/*
+ * Append txt to sb with the escaping html_url_path writes, for a url that is
+ * assembled before it reaches a sink. Nothing is captured, so it is safe to
+ * call while a capture is open.
+ */
+extern void cgit_add_url_path(struct strbuf *sb, 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);
diff --git a/source/ui-shared.c b/source/ui-shared.cindex f762fd7..409ad40 100644--- a/source/ui-shared.c+++ b/source/ui-shared.c@@ -651,11 +651,8 @@ static void print_repo_tabs(void) static void print_repo_search(void) { 	html("<form class='search' role='search' method='get' action='");-	if (ctx.cfg.virtual_root) {-		char *fileurl = cgit_fileurl(ctx.qry.repo, "log", ctx.qry.vpath, NULL);-		html_url_path(fileurl);-		free(fileurl);-	}+	if (ctx.cfg.virtual_root)+		write_page_url("log", ctx.qry.vpath); 	html("'>\n"); 	cgit_add_hidden_formfields(1, 0, "log"); 	html("<select name='qt' aria-label='Search type'>\n");@@ -896,10 +893,17 @@ const char *cgit_loginurl(void)  char *cgit_repourl(const char *reponame) {-	if (ctx.cfg.virtual_root)-		return cgit_fmtalloc("%s%s/", ctx.cfg.virtual_root, reponame);-	else-		return cgit_fmtalloc("?r=%s", reponame);+	struct strbuf sb = STRBUF_INIT;++	if (ctx.cfg.virtual_root) {+		strbuf_addstr(&sb, ctx.cfg.virtual_root);+		cgit_add_url_path(&sb, reponame);+		strbuf_addch(&sb, '/');+	} else {+		strbuf_addstr(&sb, "?r=");+		cgit_add_url_path(&sb, reponame);+	}+	return strbuf_detach(&sb, NULL); }  char *cgit_fileurl(const char *reponame, const char *pagename, const char *filename,@@ -908,16 +912,17 @@ char *cgit_fileurl(const char *reponame, const char *pagename, const char *filen 	struct strbuf sb = STRBUF_INIT; 	const char *delim; -	// The result is a raw URL, so the query joiner is a bare ampersand-	// and each sink escapes the whole string for wherever it lands.+	// The query joiner is a bare ampersand, because each sink escapes the+	// whole string for wherever it lands. 	if (ctx.cfg.virtual_root) {-		strbuf_addf(&sb, "%s%s/%s/%s", ctx.cfg.virtual_root, reponame, pagename,-			filename ? filename : "");+		strbuf_addstr(&sb, ctx.cfg.virtual_root); 		delim = "?"; 	} else {-		strbuf_addf(&sb, "?url=%s/%s/%s", reponame, pagename, (filename ? filename : ""));+		strbuf_addstr(&sb, "?url="); 		delim = "&"; 	}+	cgit_add_url_path(&sb, reponame);+	strbuf_addf(&sb, "/%s/%s", pagename, filename ? filename : ""); 	if (query) 		strbuf_addf(&sb, "%s%s", delim, query); 	return strbuf_detach(&sb, NULL);@@ -1509,7 +1514,7 @@ 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);+	html_header_query(url); 	if (query) 		html_header_query(query); 	html("\n\n");@@ -1580,18 +1585,13 @@ void cgit_print_docstart(void) 		html("'>\n"); 	} 	if (host && ctx.repo && ctx.qry.head) {-		char *fileurl;-		struct strbuf sb = STRBUF_INIT;-		strbuf_addf(&sb, "h=%s", ctx.qry.head);- 		html("<link rel='alternate' title='Atom feed' href='"); 		html(cgit_httpscheme()); 		html_attr(host);-		fileurl = cgit_fileurl(ctx.repo->url, "atom", ctx.qry.vpath, sb.buf);-		html_attr(fileurl);+		html(write_page_url("atom", ctx.qry.vpath) ? "&amp;" : "?");+		html("h=");+		html_url_arg(ctx.qry.head); 		html("' type='application/atom+xml'>\n");-		strbuf_release(&sb);-		free(fileurl); 	} 	if (ctx.repo) 		cgit_add_clone_urls(print_rel_vcs_link);
diff --git a/source/ui-shared.c b/source/ui-shared.c
index f762fd7..409ad40 100644
--- a/source/ui-shared.c
+++ b/source/ui-shared.c
@@ -651,11 +651,8 @@ static void print_repo_tabs(void)
 static void print_repo_search(void)
 {
 	html("<form class='search' role='search' method='get' action='");
-	if (ctx.cfg.virtual_root) {
-		char *fileurl = cgit_fileurl(ctx.qry.repo, "log", ctx.qry.vpath, NULL);
-		html_url_path(fileurl);
-		free(fileurl);
-	}
+	if (ctx.cfg.virtual_root)
+		write_page_url("log", ctx.qry.vpath);
 	html("'>\n");
 	cgit_add_hidden_formfields(1, 0, "log");
 	html("<select name='qt' aria-label='Search type'>\n");
@@ -896,10 +893,17 @@ const char *cgit_loginurl(void)
 
 char *cgit_repourl(const char *reponame)
 {
-	if (ctx.cfg.virtual_root)
-		return cgit_fmtalloc("%s%s/", ctx.cfg.virtual_root, reponame);
-	else
-		return cgit_fmtalloc("?r=%s", reponame);
+	struct strbuf sb = STRBUF_INIT;
+
+	if (ctx.cfg.virtual_root) {
+		strbuf_addstr(&sb, ctx.cfg.virtual_root);
+		cgit_add_url_path(&sb, reponame);
+		strbuf_addch(&sb, '/');
+	} else {
+		strbuf_addstr(&sb, "?r=");
+		cgit_add_url_path(&sb, reponame);
+	}
+	return strbuf_detach(&sb, NULL);
 }
 
 char *cgit_fileurl(const char *reponame, const char *pagename, const char *filename,
@@ -908,16 +912,17 @@ char *cgit_fileurl(const char *reponame, const char *pagename, const char *filen
 	struct strbuf sb = STRBUF_INIT;
 	const char *delim;
 
-	// The result is a raw URL, so the query joiner is a bare ampersand
-	// and each sink escapes the whole string for wherever it lands.
+	// The query joiner is a bare ampersand, because each sink escapes the
+	// whole string for wherever it lands.
 	if (ctx.cfg.virtual_root) {
-		strbuf_addf(&sb, "%s%s/%s/%s", ctx.cfg.virtual_root, reponame, pagename,
-			filename ? filename : "");
+		strbuf_addstr(&sb, ctx.cfg.virtual_root);
 		delim = "?";
 	} else {
-		strbuf_addf(&sb, "?url=%s/%s/%s", reponame, pagename, (filename ? filename : ""));
+		strbuf_addstr(&sb, "?url=");
 		delim = "&";
 	}
+	cgit_add_url_path(&sb, reponame);
+	strbuf_addf(&sb, "/%s/%s", pagename, filename ? filename : "");
 	if (query)
 		strbuf_addf(&sb, "%s%s", delim, query);
 	return strbuf_detach(&sb, NULL);
@@ -1509,7 +1514,7 @@ 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);
+	html_header_query(url);
 	if (query)
 		html_header_query(query);
 	html("\n\n");
@@ -1580,18 +1585,13 @@ void cgit_print_docstart(void)
 		html("'>\n");
 	}
 	if (host && ctx.repo && ctx.qry.head) {
-		char *fileurl;
-		struct strbuf sb = STRBUF_INIT;
-		strbuf_addf(&sb, "h=%s", ctx.qry.head);
-
 		html("<link rel='alternate' title='Atom feed' href='");
 		html(cgit_httpscheme());
 		html_attr(host);
-		fileurl = cgit_fileurl(ctx.repo->url, "atom", ctx.qry.vpath, sb.buf);
-		html_attr(fileurl);
+		html(write_page_url("atom", ctx.qry.vpath) ? "&amp;" : "?");
+		html("h=");
+		html_url_arg(ctx.qry.head);
 		html("' type='application/atom+xml'>\n");
-		strbuf_release(&sb);
-		free(fileurl);
 	}
 	if (ctx.repo)
 		cgit_add_clone_urls(print_rel_vcs_link);
diff --git a/source/ui-shared.h b/source/ui-shared.hindex fdfdbed..5a1e5fc 100644--- a/source/ui-shared.h+++ b/source/ui-shared.h@@ -25,6 +25,13 @@ extern char *cgit_currenturl(void); extern char *cgit_currentfullurl(void); extern char *cgit_currentquery(void); extern const char *cgit_loginurl(void);++/*+ * The urls of a repository and of its pages. The repository url is encoded+ * here, since it is configuration or a scanned path and may hold any byte,+ * while a page name, file name or query must arrive encoded already. The+ * result still needs escaping for wherever it lands.+ */ extern char *cgit_repourl(const char *reponame); extern void cgit_print_clone_row(const char *url, int colspan); extern char *cgit_fileurl(const char *reponame, const char *pagename, const char *filename,@@ -96,8 +103,9 @@ 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); /*- * Answer with a redirect to url, followed by query when one is given, which- * is written as it arrived.+ * Answer with a redirect to url, followed by query when one is given. Both+ * are written as they arrived, encoded already, the url the way cgit_repourl+ * builds it, and only a byte a header line cannot carry is encoded here.  */ extern void cgit_redirect(const char *url, const char *query, bool permanent); extern void cgit_print_docstart(void);
diff --git a/source/ui-shared.h b/source/ui-shared.h
index fdfdbed..5a1e5fc 100644
--- a/source/ui-shared.h
+++ b/source/ui-shared.h
@@ -25,6 +25,13 @@ extern char *cgit_currenturl(void);
 extern char *cgit_currentfullurl(void);
 extern char *cgit_currentquery(void);
 extern const char *cgit_loginurl(void);
+
+/*
+ * The urls of a repository and of its pages. The repository url is encoded
+ * here, since it is configuration or a scanned path and may hold any byte,
+ * while a page name, file name or query must arrive encoded already. The
+ * result still needs escaping for wherever it lands.
+ */
 extern char *cgit_repourl(const char *reponame);
 extern void cgit_print_clone_row(const char *url, int colspan);
 extern char *cgit_fileurl(const char *reponame, const char *pagename, const char *filename,
@@ -96,8 +103,9 @@ 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);
 /*
- * Answer with a redirect to url, followed by query when one is given, which
- * is written as it arrived.
+ * Answer with a redirect to url, followed by query when one is given. Both
+ * are written as they arrived, encoded already, the url the way cgit_repourl
+ * builds it, and only a byte a header line cannot carry is encoded here.
  */
 extern void cgit_redirect(const char *url, const char *query, bool permanent);
 extern void cgit_print_docstart(void);
diff --git a/tests/t0101-index.sh b/tests/t0101-index.shindex b2ec813..adad6be 100755--- a/tests/t0101-index.sh+++ b/tests/t0101-index.sh@@ -16,8 +16,8 @@ test_expect_success 'find foo description' 'grep "\[no description\]" tmp' test_expect_success 'find bar repo' 'grep "bar" tmp' test_expect_success 'find bar description' 'grep "the bar repo" tmp' test_expect_success 'find foo+bar repo' 'grep ">foo+bar<" tmp'-test_expect_success 'verify foo+bar link' 'grep "/foo+bar/" tmp'-test_expect_success 'verify "with%20space" link' 'grep "/with%20space/" tmp'+test_expect_success 'verify foo+bar links' 'test $(grep -o "href=./foo%2Bbar/." tmp | wc -l) = 2'+test_expect_success 'verify "with%20space" links' 'test $(grep -o "href=./with%20space/." tmp | wc -l) = 2' test_expect_success 'no tree-link' '! grep "foo/tree" tmp' test_expect_success 'no log-link' '! grep "foo/log" tmp' @@ -47,4 +47,10 @@ test_expect_success 'each css and js entry gets a link of its own' ' 	grep "/b.js" scripts ' +test_expect_success 'the feed link encodes the repository and the branch' '+	sed "s/^cache-size=.*/cache-size=0/" cgitrc >feedrc &&+	CGIT_CONFIG="$PWD/feedrc" HTTP_HOST=example.org QUERY_STRING="url=foo%2Bbar/log/&h=1%2B2" cgit >tmp &&+	grep "title=.Atom feed. href=.http://example.org/foo%2Bbar/atom/?h=1%2B2." tmp+'+ test_done
diff --git a/tests/t0101-index.sh b/tests/t0101-index.sh
index b2ec813..adad6be 100755
--- a/tests/t0101-index.sh
+++ b/tests/t0101-index.sh
@@ -16,8 +16,8 @@ test_expect_success 'find foo description' 'grep "\[no description\]" tmp'
 test_expect_success 'find bar repo' 'grep "bar" tmp'
 test_expect_success 'find bar description' 'grep "the bar repo" tmp'
 test_expect_success 'find foo+bar repo' 'grep ">foo+bar<" tmp'
-test_expect_success 'verify foo+bar link' 'grep "/foo+bar/" tmp'
-test_expect_success 'verify "with%20space" link' 'grep "/with%20space/" tmp'
+test_expect_success 'verify foo+bar links' 'test $(grep -o "href=./foo%2Bbar/." tmp | wc -l) = 2'
+test_expect_success 'verify "with%20space" links' 'test $(grep -o "href=./with%20space/." tmp | wc -l) = 2'
 test_expect_success 'no tree-link' '! grep "foo/tree" tmp'
 test_expect_success 'no log-link' '! grep "foo/log" tmp'
 
@@ -47,4 +47,10 @@ test_expect_success 'each css and js entry gets a link of its own' '
 	grep "/b.js" scripts
 '
 
+test_expect_success 'the feed link encodes the repository and the branch' '
+	sed "s/^cache-size=.*/cache-size=0/" cgitrc >feedrc &&
+	CGIT_CONFIG="$PWD/feedrc" HTTP_HOST=example.org QUERY_STRING="url=foo%2Bbar/log/&h=1%2B2" cgit >tmp &&
+	grep "title=.Atom feed. href=.http://example.org/foo%2Bbar/atom/?h=1%2B2." tmp
+'
+
 test_done
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.shindex 888ca7f..2e0e849 100755--- a/tests/t0303-robustness.sh+++ b/tests/t0303-robustness.sh@@ -639,6 +639,12 @@ test_expect_success 'the about redirect keeps the query string' ' 	grep "^Status: 200" tmp ' +test_expect_success 'the about redirect encodes the repository url once' '+	cgit_url "foo%2Bbar/about/" >tmp &&+	grep "^Status: 302" tmp &&+	grep "^Location: /foo%2Bbar/$" tmp+'+ # The about page shows the readme alone and its links lead into the tree, # so a path below it names nothing, the site about page included. test_expect_success 'a path below the about page answers 404' '
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.sh
index 888ca7f..2e0e849 100755
--- a/tests/t0303-robustness.sh
+++ b/tests/t0303-robustness.sh
@@ -639,6 +639,12 @@ test_expect_success 'the about redirect keeps the query string' '
 	grep "^Status: 200" tmp
 '
 
+test_expect_success 'the about redirect encodes the repository url once' '
+	cgit_url "foo%2Bbar/about/" >tmp &&
+	grep "^Status: 302" tmp &&
+	grep "^Location: /foo%2Bbar/$" tmp
+'
+
 # The about page shows the readme alone and its links lead into the tree,
 # so a path below it names nothing, the site about page included.
 test_expect_success 'a path below the about page answers 404' '