6 files changed, 67 insertions, 27 deletions
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.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.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.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.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.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' '