On 2022/09/03 11:27:58 +0200, Omar Polo <op@omarpolo.com> wrote:
> TL;DR: wanna break gotwebd?
> 
> 	$ touch "' onclick='alert(\"p0wn3d\");' foo='"
> 	$ got add -R .
> 	$ got commit -m ...
> 	$ got send
> 
> enjoy!
> 
> (this doesn't work on modern browsers due to the CSP, but gives the
> idea.)
> 
> 
> Longer version: gotwebd doesn't encode the arguments in the generated
> URLs.  This is a companion diff to the other diff i've sent ("gotwebd:
> urldecode querystring") and deals with how URLs are generated.
> 
> Some URL parameters needs to be encoded as they contain data that may
> otherwise break the HTML.  Among these `folder', `file' and `path'.
> Instead of making the current URL construction even more complex, I
> decided to take it as a chance to refactor a bit how we generate URLs.
> 
> Diff below introduces a new helper: `gotweb_link'.  It takes the
> request, a struct gotweb_url that represent the request, followed by a
> printf-format string and arguments.  It makes gotwebd gain a few lines
> of codes because now the various fcgi_printf calls needs to be
> splitted, but otherwise I think it's an improvement.
> 
> What do y'all think?  Is it OK to use the syntax
> 
> 	r = gotweb_link(c, &(struct gotweb_url){
> 		.index_page = -1,
> 		.action = DIFF,
> 		.page = -1,
> 		.path = repo_dir->name,
> 		.commit = bline->id_str,
> 	    }, "%.8s", bline->id_str);
> 
> to pass an anounymous struct pointer?

here's a rebased and slightly improved diff.  it makes gotwebd gains a
few lines of code but I think it's better than revisit every function
where we print a link and allocate yet another local string that we
might forget to free and making the output functions even more complex
to follow.  it also centralize how we generate URLs, hopefully making
the life easier in the future if we want to change things.

the changes to the previous version are:

 - escape the `headref' parametr too
 - use a consistent ordering of the fields
 - add a comment before the gotweb_url struct

diff refs/heads/main refs/heads/%enc
commit - 8ee99f946108c3442fcc98fa53ecc9264ed5d947
commit + 34cef9dcccb49096963b96eac94f884f62cd9a45
blob - e77b8424231bf88c153a32dd90a19292c41dc39d
blob + 04e9e72ece31adeacc0aef6f6fb7cffbaace078a
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -288,16 +288,12 @@ fcgi_timeout(int fd, short events, void *arg)
 }
 
 int
-fcgi_printf(struct request *c, const char *fmt, ...)
+fcgi_vprintf(struct request *c, const char *fmt, va_list ap)
 {
-	va_list ap;
 	char *str;
 	int r;
 
-	va_start(ap, fmt);
 	r = vasprintf(&str, fmt, ap);
-	va_end(ap);
-
 	if (r == -1) {
 		log_warn("%s: asprintf", __func__);
 		return -1;
@@ -309,6 +305,19 @@ fcgi_printf(struct request *c, const char *fmt, ...)
 }
 
 int
+fcgi_printf(struct request *c, const char *fmt, ...)
+{
+	va_list ap;
+	int r;
+
+	va_start(ap, fmt);
+	r = fcgi_vprintf(c, fmt, ap);
+	va_end(ap);
+
+	return r;
+}
+
+int
 fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len)
 {
 	int r;
blob - 04dc980392274b236587daf89d34a45c992b64c1
blob + 5211e085ce0097066a9ec6cb94c843ffc952f29f
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -810,7 +810,7 @@ got_output_repo_tree(struct request *c)
 	struct got_reflist_head refs;
 	struct got_tree_object *tree = NULL;
 	struct repo_dir *repo_dir = t->repo_dir;
-	const char *name, *index_page_str, *folder;
+	const char *name, *folder;
 	char *escaped_name = NULL, *path = NULL;
 	int nentries, i, r;
 
@@ -849,7 +849,6 @@ got_output_repo_tree(struct request *c)
 
 	nentries = got_object_tree_get_nentries(tree);
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
 	folder = qs->folder ? qs->folder : "";
 
 	for (i = 0; i < nentries; i++) {
@@ -877,42 +876,76 @@ got_output_repo_tree(struct request *c)
 			goto done;
 
 		if (S_ISDIR(mode)) {
-			r = fcgi_printf(c,
-			    "<div class='tree_wrapper'>\n"
-			    "<div class='tree_line'>"
-			    "<a class='diff_directory' "
-			    "href='?index_page=%s&path=%s&action=tree"
-			    "&commit=%s&folder=%s/%s'>%s%s</a>"
-			    "</div>\n" /* .tree_line */
+			struct gotweb_url url = {
+				.index_page = -1,
+				.page = -1,
+				.action = TREE,
+				.commit = rc->commit_id,
+				.path = qs->path,
+				/* `folder' is filled later */
+			};
+			char *path = NULL;
+
+			if (fcgi_printf(c,"<div class='tree_wrapper'>\n"
+			    "<div class='tree_line'>") == -1)
+				goto done;
+
+			if (asprintf(&path, "%s/%s", folder, name) == -1) {
+				error = got_error_from_errno("asprintf");
+				goto done;
+			}
+			url.folder = path;
+			r = gotweb_link(c, &url, "%s%s", escaped_name,
+			    modestr);
+			free(path);
+			if (r == -1)
+				goto done;
+
+			if (fcgi_printf(c, "</div>\n" /* .tree_line */
 			    "<div class='tree_line_blank'>&nbsp;</div>\n"
-			    "</div>\n", /* .tree_wrapper */
-			    index_page_str, qs->path, rc->commit_id,
-			    folder, name, escaped_name, modestr);
-			if (r == -1)
+			    "</div>\n") == -1)
 				goto done;
 		} else {
-			r = fcgi_printf(c,
-			    "<div class='tree_wrapper'>\n"
-			    "<div class='tree_line'>"
-			    "<a href='?index_page=%s&path=%s&action=blob"
-			    "&commit=%s&folder=%s&file=%s'>%s%s</a>"
-			    "</div>\n" /* .tree_line */
-			    "<div class='tree_line_blank'>"
-			    "<a href='?index_page=%s&path=%s&action=commits"
-			    "&commit=%s&folder=%s&file=%s'>commits</a>\n"
-			    " | \n"
-			    "<a href='?index_page=%s&path=%s&action=blame"
-			    "&commit=%s&folder=%s&file=%s'>blame</a>\n"
-			    "</div>\n"  /* .tree_line_blank */
-			    "</div>\n", /* .tree_wrapper */
-			    index_page_str, qs->path, rc->commit_id,
-			    folder, name, escaped_name, modestr,
-			    index_page_str, qs->path, rc->commit_id,
-			    folder, name,
-			    index_page_str, qs->path, rc->commit_id,
-			    folder, name);
+			struct gotweb_url url = {
+				.index_page = -1,
+				.page = -1,
+				.path = qs->path,
+				.commit = rc->commit_id,
+				.folder = folder,
+				.file = name,
+			};
+
+			if (fcgi_printf(c, "<div class='tree_wrapper'>\n"
+			    "<div class='tree_line'>") == -1)
+				goto done;
+
+			url.action = BLOB;
+			r = gotweb_link(c, &url, "%s%s", escaped_name,
+			    modestr);
 			if (r == -1)
 				goto done;
+
+			if (fcgi_printf(c, "</div>\n" /* .tree_line */
+			    "<div class='tree_line_blank'>") == -1)
+				goto done;
+
+			url.action = COMMITS;
+			r = gotweb_link(c, &url, "commits");
+			if (r == -1)
+				goto done;
+
+			if (fcgi_printf(c, " | ") == -1)
+				goto done;
+
+			url.action = BLAME;
+			r = gotweb_link(c, &url, "blame");
+			if (r == -1)
+				goto done;
+
+			if (fcgi_printf(c,
+			    "</div>\n"		/* .tree_line_blank */
+			    "</div>\n") == -1)	/* .tree_wrapper */
+				goto done;
 		}
 		free(escaped_name);
 		escaped_name = NULL;
@@ -1069,9 +1102,7 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno,
 	struct blame_line *bline;
 	struct request *c = a->c;
 	struct transport *t = c->t;
-	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
-	const char *index_page_str;
 	char *line = NULL, *eline = NULL;
 	size_t linesize = 0;
 	off_t offset;
@@ -1121,8 +1152,6 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno,
 		goto done;
 	}
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	while (a->lineno_cur <= a->nlines && bline->annotated) {
 		char *smallerthan, *at, *nl, *committer;
 		size_t len;
@@ -1152,19 +1181,28 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno,
 		if (err)
 			goto done;
 
-		r = fcgi_printf(c, "<div class='blame_wrapper'>"
+		if (fcgi_printf(c, "<div class='blame_wrapper'>"
 		    "<div class='blame_number'>%.*d</div>"
-		    "<div class='blame_hash'>"
-		    "<a href='?index_page=%s&path=%s&action=diff&commit=%s'>"
-		    "%.8s</a>"
+		    "<div class='blame_hash'>",
+		    a->nlines_prec, a->lineno_cur) == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = DIFF,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = bline->id_str,
+		    }, "%.8s", bline->id_str);
+		if (r == -1)
+			goto done;
+
+		r = fcgi_printf(c,
 		    "</div>"
 		    "<div class='blame_date'>%s</div>"
 		    "<div class='blame_author'>%s</div>"
 		    "<div class='blame_code'>%s</div>"
 		    "</div>",	/* .blame_wrapper */
-		    a->nlines_prec, a->lineno_cur,
-		    index_page_str, repo_dir->name, bline->id_str,
-		    bline->id_str,
 		    bline->datebuf,
 		    committer,
 		    eline);
blob - 1312e2683bc2b1ff818d0c837d1b1e4ce670ab62
blob + 5e13cb4f5f40682282ea17f7ab4440076a52815b
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -694,6 +694,7 @@ gotweb_render_content_type_file(struct request *c, con
 static const struct got_error *
 gotweb_render_header(struct request *c)
 {
+	const struct got_error *err = NULL;
 	struct server *srv = c->srv;
 	struct querystring *qs = c->t->qs;
 	int r;
@@ -743,10 +744,21 @@ gotweb_render_header(struct request *c)
 
 	if (qs != NULL) {
 		if (qs->path != NULL) {
-			r = fcgi_printf(c, " / "
-			    "<a href='?index_page=%d&path=%s&action=summary'>"
-			    "%s</a>",
-			    qs->index_page, qs->path, qs->path);
+			char *epath;
+
+			if (fcgi_printf(c, " / ") == -1)
+				goto done;
+
+			err = gotweb_escape_html(&epath, qs->path);
+			if (err)
+				return err;
+			r = gotweb_link(c, &(struct gotweb_url){
+				.action = SUMMARY,
+				.index_page = -1,
+				.page = -1,
+				.path = qs->path,
+			    }, "%s", epath);
+			free(epath);
 			if (r == -1)
 				goto done;
 		}
@@ -826,8 +838,7 @@ gotweb_render_navs(struct request *c)
 	struct transport *t = c->t;
 	struct querystring *qs = t->qs;
 	struct server *srv = c->srv;
-	char *nhref = NULL, *phref = NULL;
-	int r, disp = 0;
+	int r;
 
 	r = fcgi_printf(c, "<div id='np_wrapper'>\n<div id='nav_prev'>\n");
 	if (r == -1)
@@ -836,140 +847,134 @@ gotweb_render_navs(struct request *c)
 	switch(qs->action) {
 	case INDEX:
 		if (qs->index_page > 0) {
-			if (asprintf(&phref, "index_page=%d",
-			    qs->index_page - 1) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = -1,
+				.index_page = qs->index_page - 1,
+				.page = -1,
+			};
+
+			r = gotweb_link(c, &url, "Previous");
 		}
 		break;
 	case BRIEFS:
 		if (t->prev_id && qs->commit != NULL &&
 		    strcmp(qs->commit, t->prev_id) != 0) {
-			if (asprintf(&phref, "index_page=%d&path=%s&page=%d"
-			    "&action=briefs&commit=%s&headref=%s",
-			    qs->index_page, qs->path, qs->page - 1, t->prev_id,
-			    qs->headref) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = BRIEFS,
+				.index_page = -1,
+				.page = qs->page - 1,
+				.path = qs->path,
+				.commit = t->prev_id,
+				.headref = qs->headref,
+			};
+
+			r = gotweb_link(c, &url, "Previous");
 		}
 		break;
 	case COMMITS:
 		if (t->prev_id && qs->commit != NULL &&
 		    strcmp(qs->commit, t->prev_id) != 0) {
-			if (asprintf(&phref, "index_page=%d&path=%s&page=%d"
-			    "&action=commits&commit=%s&headref=%s&folder=%s"
-			    "&file=%s",
-			    qs->index_page, qs->path, qs->page - 1, t->prev_id,
-			    qs->headref, qs->folder ? qs->folder : "",
-			    qs->file ? qs->file : "") == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = COMMIT,
+				.index_page = -1,
+				.page = qs->page - 1,
+				.path = qs->path,
+				.commit = t->prev_id,
+				.headref = qs->headref,
+				.folder = qs->folder,
+				.file = qs->file,
+			};
+
+			r = gotweb_link(c, &url, "Previous");
 		}
 		break;
 	case TAGS:
 		if (t->prev_id && qs->commit != NULL &&
 		    strcmp(qs->commit, t->prev_id) != 0) {
-			if (asprintf(&phref, "index_page=%d&path=%s&page=%d"
-			    "&action=tags&commit=%s&headref=%s",
-			    qs->index_page, qs->path, qs->page - 1, t->prev_id,
-			    qs->headref) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = TAGS,
+				.index_page = -1,
+				.page = qs->page - 1,
+				.path = qs->path,
+				.commit = t->prev_id,
+				.headref = qs->headref,
+			};
+
+			r = gotweb_link(c, &url, "Previous");
 		}
 		break;
-	default:
-		disp = 0;
-		break;
 	}
 
-	if (disp) {
-		r = fcgi_printf(c, "<a href='?%s'>Previous</a>", phref);
-		if (r == -1)
-			goto done;
-	}
+	if (r == -1)
+		goto done;
 
 	r = fcgi_printf(c, "</div>\n"	/* #nav_prev */
 	    "<div id='nav_next'>");
 	if (r == -1)
 		goto done;
 
-	disp = 0;
 	switch(qs->action) {
 	case INDEX:
 		if (t->next_disp == srv->max_repos_display &&
 		    t->repos_total != (qs->index_page + 1) *
 		    srv->max_repos_display) {
-			if (asprintf(&nhref, "index_page=%d",
-			    qs->index_page + 1) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = -1,
+				.index_page = qs->index_page + 1,
+				.page = -1,
+			};
+
+			r = gotweb_link(c, &url, "Next");
 		}
 		break;
 	case BRIEFS:
 		if (t->next_id) {
-			if (asprintf(&nhref, "index_page=%d&path=%s&page=%d"
-			    "&action=briefs&commit=%s&headref=%s",
-			    qs->index_page, qs->path, qs->page + 1, t->next_id,
-			    qs->headref) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = BRIEFS,
+				.index_page = -1,
+				.page = qs->page + 1,
+				.path = qs->path,
+				.commit = t->next_id,
+				.headref = qs->headref,
+			};
+
+			r = gotweb_link(c, &url, "Next");
 		}
 		break;
 	case COMMITS:
 		if (t->next_id) {
-			if (asprintf(&nhref, "index_page=%d&path=%s&page=%d"
-			    "&action=commits&commit=%s&headref=%s&folder=%s"
-			    "&file=%s",
-			    qs->index_page, qs->path, qs->page + 1, t->next_id,
-			    qs->headref, qs->folder ? qs->folder : "",
-			    qs->file ? qs->file : "") == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = COMMIT,
+				.index_page = -1,
+				.page = qs->page + 1,
+				.path = qs->path,
+				.commit = t->next_id,
+				.headref = qs->headref,
+				.folder = qs->folder,
+				.file = qs->file,
+			};
+
+			r = gotweb_link(c, &url, "Next");
 		}
 		break;
 	case TAGS:
 		if (t->next_id) {
-			if (asprintf(&nhref, "index_page=%d&path=%s&page=%d"
-			    "&action=tags&commit=%s&headref=%s",
-			    qs->index_page, qs->path, qs->page + 1, t->next_id,
-			    qs->headref) == -1) {
-				error = got_error_from_errno2("%s: asprintf",
-				    __func__);
-				goto done;
-			}
-			disp = 1;
+			struct gotweb_url url = {
+				.action = TAGS,
+				.index_page = -1,
+				.page = qs->page + 1,
+				.path = qs->path,
+				.commit = t->next_id,
+				.headref = qs->headref,
+			};
+
+			r = gotweb_link(c, &url, "Next");
 		}
 		break;
-	default:
-		disp = 0;
-		break;
 	}
-	if (disp) {
-		r = fcgi_printf(c, "<a href='?%s'>Next</a>", nhref);
-		if (r == -1)
-			goto done;
-	}
+	if (r == -1)
+		goto done;
+
 	fcgi_printf(c, "</div>\n"); /* #nav_next */
 	fcgi_printf(c, "</div>\n"); /* #np_wrapper */
 done:
@@ -977,8 +982,6 @@ done:
 	t->next_id = NULL;
 	free(t->prev_id);
 	t->prev_id = NULL;
-	free(phref);
-	free(nhref);
 	return error;
 }
 
@@ -992,14 +995,11 @@ gotweb_render_index(struct request *c)
 	struct repo_dir *repo_dir = NULL;
 	DIR *d;
 	struct dirent **sd_dent = NULL;
-	const char *index_page_str;
 	char *c_path = NULL;
 	struct stat st;
 	unsigned int d_cnt, d_i, d_disp = 0;
 	int r;
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	d = opendir(srv->repos_path);
 	if (d == NULL) {
 		error = got_error_from_errno2("opendir", srv->repos_path);
@@ -1091,17 +1091,22 @@ render:
 		d_disp++;
 		t->prev_disp++;
 
-		r = fcgi_printf(c, "<div class='index_wrapper'>\n"
-		    "<div class='index_project'>"
-		    "<a href='?index_page=%s&path=%s&action=summary'>"
-		    " %s "
-		    "</a>"
-		    "</div>",	/* .index_project */
-		    index_page_str, repo_dir->name,
-		    repo_dir->name);
+		if (fcgi_printf(c, "<div class='index_wrapper'>\n"
+		    "<div class='index_project'>") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = SUMMARY,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+		    }, "%s", repo_dir->name);
 		if (r == -1)
 			goto done;
 
+		if (fcgi_printf(c, "</div>") == -1) /* .index_project */
+			goto done;
+
 		if (srv->show_repo_description) {
 			r = fcgi_printf(c,
 			    "<div class='index_project_description'>\n"
@@ -1124,32 +1129,71 @@ render:
 				goto done;
 		}
 
-		r = fcgi_printf(c, "<div class='navs_wrapper'>"
-		    "<div class='navs'>"
-		    "<a href='?index_page=%s&path=%s&action=summary'>"
-		    "summary"
-		    "</a> | "
-		    "<a href='?index_page=%s&path=%s&action=briefs'>"
-		    "commit briefs"
-		    "</a> | "
-		    "<a href='?index_page=%s&path=%s&action=commits'>"
-		    "commits"
-		    "</a> | "
-		    "<a href='?index_page=%s&path=%s&action=tags'>"
-		    "tags"
-		    "</a> | "
-		    "<a href='?index_page=%s&path=%s&action=tree'>"
-		    "tree"
-		    "</a>"
-		    "</div>"	/* .navs */
+		if (fcgi_printf(c, "<div class='navs_wrapper'>"
+		    "<div class='navs'>") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = SUMMARY,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name
+		    }, "summary");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = BRIEFS,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name
+		    }, "commit briefs");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = COMMITS,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name
+		    }, "commits");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TAGS,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name
+		    }, "tags");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TREE,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name
+		    }, "tree");
+		if (r == -1)
+			goto done;
+
+		r = fcgi_printf(c, "</div>"	/* .navs */
 		    "<div class='dotted_line'></div>\n"
-		    "</div>\n"	/* .navs_wrapper */
-		    "</div>\n",	/* .index_wrapper */
-		    index_page_str, repo_dir->name,
-		    index_page_str, repo_dir->name,
-		    index_page_str, repo_dir->name,
-		    index_page_str, repo_dir->name,
-		    index_page_str, repo_dir->name);
+		    "</div>\n"			/* .navs_wrapper */
+		    "</div>\n");		/* .index_wrapper */
 		if (r == -1)
 			goto done;
 
@@ -1243,13 +1287,10 @@ gotweb_render_briefs(struct request *c)
 	struct transport *t = c->t;
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
-	const char *index_page_str;
 	char *smallerthan, *newline;
 	char *age = NULL, *author = NULL, *msg = NULL;
 	int r;
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	r = fcgi_printf(c, "<div id='briefs_title_wrapper'>\n"
 	    "<div id='briefs_title'>Commit Briefs</div>\n"
 	    "</div>\n"	/* #briefs_title_wrapper */
@@ -1287,16 +1328,22 @@ gotweb_render_briefs(struct request *c)
 
 		r = fcgi_printf(c, "<div class='briefs_age'>%s</div>\n"
 		    "<div class='briefs_author'>%s</div>\n"
-		    "<div class='briefs_log'>"
-		    "<a href='?index_page=%s&path=%s&action=diff&commit=%s"
-		    "&headref=%s'>%s</a>",
-		    age,
-		    author,
-		    index_page_str, repo_dir->name, rc->commit_id, qs->headref,
-		    msg);
+		    "<div class='briefs_log'>",
+		    age, author);
 		if (r == -1)
 			goto done;
 
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = DIFF,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rc->commit_id,
+			.headref = qs->headref,
+		    }, "%s", msg);
+		if (r == -1)
+			goto done;
+
 		if (rc->refs_str) {
 			char *refs;
 
@@ -1313,18 +1360,38 @@ gotweb_render_briefs(struct request *c)
 			goto done;
 
 		r = fcgi_printf(c, "<div class='navs_wrapper'>\n"
-		    "<div class='navs'>"
-		    "<a href='?index_page=%s&path=%s&action=diff&commit=%s"
-		    "&headref=%s'>diff</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=tree&commit=%s"
-		    "&headref=%s'>tree</a>"
-		    "</div>\n"	/* .navs */
+		    "<div class='navs'>");
+		if (r == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = DIFF,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rc->commit_id,
+			.headref = qs->headref,
+		    }, "diff");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TREE,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rc->commit_id,
+			.headref = qs->headref,
+		    }, "tree");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, "</div>\n"	/* .navs */
 		    "</div>\n"	/* .navs_wrapper */
-		    "<div class='dotted_line'></div>\n",
-		    index_page_str, repo_dir->name, rc->commit_id, qs->headref,
-		    index_page_str, repo_dir->name, rc->commit_id, qs->headref);
-		if (r == -1)
+		    "<div class='dotted_line'></div>\n") == -1)
 			goto done;
 
 		free(age);
@@ -1355,14 +1422,10 @@ gotweb_render_commits(struct request *c)
 	struct repo_commit *rc = NULL;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
-	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
-	const char *index_page_str;
 	char *age = NULL, *author = NULL, *msg = NULL;
 	int r;
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	r = fcgi_printf(c, "<div class='commits_title_wrapper'>\n"
 	    "<div class='commits_title'>Commits</div>\n"
 	    "</div>\n"		/* .commits_title_wrapper */
@@ -1404,19 +1467,36 @@ gotweb_render_commits(struct request *c)
 		if (r == -1)
 			goto done;
 
-		r = fcgi_printf(c, "<div class='navs_wrapper'>\n"
-		    "<div class='navs'>"
-		    "<a href='?index_page=%s&path=%s&action=diff&commit=%s'>"
-		    "diff</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=tree&commit=%s'>"
-		    "tree</a>"
-		    "</div>\n"	/* .navs */
+		if (fcgi_printf(c, "<div class='navs_wrapper'>\n"
+		    "<div class='navs'>") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = DIFF,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rc->commit_id,
+		    }, "diff");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TREE,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rc->commit_id,
+		    }, "tree");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, "</div>\n"	/* .navs */
 		    "</div>\n"	/* .navs_wrapper */
-		    "<div class='dotted_line'></div>\n",
-		    index_page_str, repo_dir->name, rc->commit_id,
-		    index_page_str, repo_dir->name, rc->commit_id);
-		if (r == -1)
+		    "<div class='dotted_line'></div>\n") == -1)
 			goto done;
 
 		free(age);
@@ -1449,12 +1529,10 @@ gotweb_render_branches(struct request *c)
 	struct transport *t = c->t;
 	struct querystring *qs = t->qs;
 	struct got_repository *repo = t->repo;
-	const char *index_page_str;
+	char *escaped_refname = NULL;
 	char *age = NULL;
 	int r;
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	TAILQ_INIT(&refs);
 
 	error = got_ref_list(&refs, repo, "refs/heads",
@@ -1471,7 +1549,6 @@ gotweb_render_branches(struct request *c)
 
 	TAILQ_FOREACH(re, &refs, entry) {
 		const char *refname = NULL;
-		char *escaped_refname = NULL;
 
 		if (got_ref_is_symbolic(re->ref))
 			continue;
@@ -1498,41 +1575,77 @@ gotweb_render_branches(struct request *c)
 		r = fcgi_printf(c, "<div class='branches_wrapper'>\n"
 		    "<div class='branches_age'>%s</div>\n"
 		    "<div class='branches_space'>&nbsp;</div>\n"
-		    "<div class='branch'>"
-		    "<a href='?index_page=%s&path=%s&action=summary&headref=%s'>"
-		    "%s</a>"
-		    "</div>\n"	/* .branch */
+		    "<div class='branch'>", age);
+		if (r == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = SUMMARY,
+			.index_page = -1,
+			.page = -1,
+			.path = qs->path,
+			.headref = refname,
+		    }, "%s", escaped_refname);
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, "</div>\n"	/* .branch */
 		    "<div class='navs_wrapper'>\n"
-		    "<div class='navs'>"
-		    "<a href='?index_page=%s&path=%s&action=summary&headref=%s'>"
-		    "summary</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=briefs&headref=%s'>"
-		    "commit briefs</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=commits&headref=%s'>"
-		    "commits</a>"
-		    "</div>\n"	/* .navs */
-		    "</div>\n"	/* .navs_wrapper */
+		    "<div class='navs'>") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = SUMMARY,
+			.index_page = -1,
+			.page = -1,
+			.path = qs->path,
+			.headref = refname,
+		    }, "summary");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = BRIEFS,
+			.index_page = -1,
+			.page = -1,
+			.path = qs->path,
+			.headref = refname,
+		    }, "commit briefs");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = COMMITS,
+			.index_page = -1,
+			.page = -1,
+			.path = qs->path,
+			.headref = refname,
+		    }, "commits");
+		if (r == -1)
+			goto done;
+
+		r = fcgi_printf(c, "</div>\n"	/* .navs */
+		    "</div>\n"			/* .navs_wrapper */
 		    "<div class='dotted_line'></div>\n"
-		    "</div>\n",	/* .branches_wrapper */
-		    age ? age : "",
-		    index_page_str, qs->path, refname,
-		    escaped_refname,
-		    index_page_str, qs->path, refname,
-		    index_page_str, qs->path, refname,
-		    index_page_str, qs->path, refname);
+		    "</div>\n");		/* .branches_wrapper */
+		if (r == -1)
+			goto done;
+
+		free(age);
+		age = NULL;
 		free(escaped_refname);
-		if (r == -1)
-			goto done;
-
-		free(age);
-		age = NULL;
-
+		escaped_refname = NULL;
 	}
 	fcgi_printf(c, "</div>\n"); /* #branches_content */
 done:
 	free(age);
+	free(escaped_refname);
 	got_ref_list_free(&refs);
 	return error;
 }
@@ -1812,12 +1925,9 @@ gotweb_render_tags(struct request *c)
 	struct transport *t = c->t;
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
-	const char *index_page_str;
 	char *age = NULL, *tagname = NULL, *msg = NULL, *newline;
 	int r, commit_found = 0;
 
-	index_page_str = qs->index_page_str ? qs->index_page_str : "";
-
 	if (qs->action == BRIEFS) {
 		qs->action = TAGS;
 		error = got_get_repo_tags(c, D_MAXSLCOMMDISP);
@@ -1867,32 +1977,66 @@ gotweb_render_tags(struct request *c)
 				goto done;
 		}
 
-		r = fcgi_printf(c, "<div class='tag_age'>%s</div>\n"
+		if (fcgi_printf(c, "<div class='tag_age'>%s</div>\n"
 		    "<div class='tag'>%s</div>\n"
-		    "<div class='tag_log'>"
-		    "<a href='?index_page=%s&path=%s&action=tag&commit=%s'>"
-		    "%s</a>"
-		    "</div>\n"	/* .tag_log */
+		    "<div class='tag_log'>", age, tagname) == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TAG,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rt->commit_id,
+		    }, "%s", msg ? msg : "");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, "</div>\n"	/* .tag_log */
 		    "<div class='navs_wrapper'>\n"
-		    "<div class='navs'>"
-		    "<a href='?index_page=%s&path=%s&action=tag&commit=%s'>"
-		    "tag</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=briefs&commit=%s'>"
-		    "commit briefs</a>"
-		    " | "
-		    "<a href='?index_page=%s&path=%s&action=commits&commit=%s'>"
-		    "commits</a>"
+		    "<div class='navs'>") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = TAG,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rt->commit_id,
+		    }, "tag");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = BRIEFS,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rt->commit_id,
+		    }, "commit briefs");
+		if (r == -1)
+			goto done;
+
+		if (fcgi_printf(c, " | ") == -1)
+			goto done;
+
+		r = gotweb_link(c, &(struct gotweb_url){
+			.action = COMMITS,
+			.index_page = -1,
+			.page = -1,
+			.path = repo_dir->name,
+			.commit = rt->commit_id,
+		    }, "commits");
+		if (r == -1)
+			goto done;
+
+		r = fcgi_printf(c,
 		    "</div>\n"	/* .navs */
 		    "</div>\n"	/* .navs_wrapper */
-		    "<div class='dotted_line'></div>\n",
-		    age,
-		    tagname,
-		    index_page_str, repo_dir->name, rt->commit_id,
-		    msg ? msg : "",
-		    index_page_str, repo_dir->name, rt->commit_id,
-		    index_page_str, repo_dir->name, rt->commit_id,
-		    index_page_str, repo_dir->name, rt->commit_id);
+		    "<div class='dotted_line'></div>\n");
 		if (r == -1)
 			goto done;
 
@@ -1980,6 +2124,223 @@ done:
 	return error;
 }
 
+static inline int
+should_urlencode(int c)
+{
+	if (c <= ' ' || c >= 127)
+		return 1;
+
+	switch (c) {
+		/* gen-delim */
+	case ':':
+	case '/':
+	case '?':
+	case '#':
+	case '[':
+	case ']':
+	case '@':
+		/* sub-delims */
+	case '!':
+	case '$':
+	case '&':
+	case '\'':
+	case '(':
+	case ')':
+	case '*':
+	case '+':
+	case ',':
+	case ';':
+	case '=':
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static char *
+gotweb_urlencode(const char *str)
+{
+	const char *s;
+	char *escaped;
+	size_t i, len;
+	int a, b;
+
+	len = 0;
+	for (s = str; *s; ++s) {
+		len++;
+		if (should_urlencode(*s))
+			len += 2;
+	}
+
+	escaped = calloc(1, len + 1);
+	if (escaped == NULL)
+		return NULL;
+
+	i = 0;
+	for (s = str; *s; ++s) {
+		if (should_urlencode(*s)) {
+			a = (*s & 0xF0) >> 4;
+			b = (*s & 0x0F);
+
+			escaped[i++] = '%';
+			escaped[i++] = a <= 9 ? ('0' + a) : ('7' + a);
+			escaped[i++] = b <= 9 ? ('0' + b) : ('7' + b);
+		} else
+			escaped[i++] = *s;
+	}
+
+	return escaped;
+}
+
+static inline const char *
+action_name(int action)
+{
+	switch (action) {
+	case BLAME:
+		return "blame";
+	case BLOB:
+		return "blob";
+	case BRIEFS:
+		return "briefs";
+	case COMMITS:
+		return "commits";
+	case DIFF:
+		return "diff";
+	case ERR:
+		return "err";
+	case INDEX:
+		return "index";
+	case SUMMARY:
+		return "summary";
+	case TAG:
+		return "tag";
+	case TAGS:
+		return "tags";
+	case TREE:
+		return "tree";
+	default:
+		return NULL;
+	}
+}
+
+static int
+gotweb_print_url(struct request *c, struct gotweb_url *url)
+{
+	const char *sep = "?", *action;
+	char *tmp;
+	int r;
+
+	action = action_name(url->action);
+	if (action != NULL) {
+		if (fcgi_printf(c, "?action=%s", action) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->commit) {
+		if (fcgi_printf(c, "%scommit=%s", sep, url->commit) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->previd) {
+		if (fcgi_printf(c, "%sprevid=%s", sep, url->previd) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->prevset) {
+		if (fcgi_printf(c, "%sprevset=%s", sep, url->prevset) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->file) {
+		tmp = gotweb_urlencode(url->file);
+		if (tmp == NULL)
+			return -1;
+		r = fcgi_printf(c, "%sfile=%s", sep, tmp);
+		free(tmp);
+		if (r == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->folder) {
+		tmp = gotweb_urlencode(url->folder);
+		if (tmp == NULL)
+			return -1;
+		r = fcgi_printf(c, "%sfolder=%s", sep, tmp);
+		free(tmp);
+		if (r == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->headref) {
+		tmp = gotweb_urlencode(url->headref);
+		if (tmp == NULL)
+			return -1;
+		r = fcgi_printf(c, "%sheadref=%s", sep, url->headref);
+		free(tmp);
+		if (r == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->index_page != -1) {
+		if (fcgi_printf(c, "%sindex_page=%d", sep,
+		    url->index_page) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->path) {
+		tmp = gotweb_urlencode(url->path);
+		if (tmp == NULL)
+			return -1;
+		r = fcgi_printf(c, "%spath=%s", sep, tmp);
+		free(tmp);
+		if (r == -1)
+			return -1;
+		sep = "&";
+	}
+
+	if (url->page != -1) {
+		if (fcgi_printf(c, "%spage=%d", sep, url->page) == -1)
+			return -1;
+		sep = "&";
+	}
+
+	return 0;
+}
+
+int
+gotweb_link(struct request *c, struct gotweb_url *url, const char *fmt, ...)
+{
+	va_list ap;
+	int r;
+
+	if (fcgi_printf(c, "<a href='") == -1)
+		return -1;
+
+	if (gotweb_print_url(c, url) == -1)
+		return -1;
+
+	if (fcgi_printf(c, "'>") == -1)
+		return -1;
+
+	va_start(ap, fmt);
+	r = fcgi_vprintf(c, fmt, ap);
+	va_end(ap);
+	if (r == -1)
+		return -1;
+
+	if (fcgi_printf(c, "</a>"))
+		return -1;
+	return 0;
+}
+
 static struct got_repository *
 find_cached_repo(struct server *srv, const char *path)
 {
blob - f73ea6b8311442f992dbc1c563f90486ef813c04
blob + 06f8d01516c1b327e8524a128e571a8cc082ca7e
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -341,6 +341,23 @@ struct gotwebd {
 	char		 unix_socket_name[PATH_MAX];
 };
 
+/*
+ * URL parameter for gotweb_link.  NULL values and int set to -1 are
+ * implicitly ignored, and string are properly escaped.
+ */
+struct gotweb_url {
+	int		 action;
+	int		 index_page;
+	int		 page;
+	const char	*commit;
+	const char	*previd;
+	const char	*prevset;
+	const char	*file;
+	const char	*folder;
+	const char	*headref;
+	const char	*path;
+};
+
 struct querystring {
 	uint8_t		 action;
 	char		*commit;
@@ -411,6 +428,9 @@ const struct got_error
 const struct got_error *gotweb_get_time_str(char **, time_t, int);
 const struct got_error *gotweb_init_transport(struct transport **);
 const struct got_error *gotweb_escape_html(char **, const char *);
+int gotweb_link(struct request *, struct gotweb_url *, const char *, ...)
+	__attribute__((__format__(printf, 3, 4)))
+	__attribute__((__nonnull__(3)));
 void gotweb_free_repo_commit(struct repo_commit *);
 void gotweb_free_repo_tag(struct repo_tag *);
 void gotweb_process_request(struct request *);
@@ -426,6 +446,7 @@ void fcgi_timeout(int, short, void *);
 void fcgi_cleanup_request(struct request *);
 void fcgi_create_end_record(struct request *);
 void dump_fcgi_record(const char *, struct fcgi_record_header *);
+int fcgi_vprintf(struct request *, const char *, va_list);
 int fcgi_printf(struct request *, const char *, ...)
 	__attribute__((__format__(printf, 2, 3)))
 	__attribute__((__nonnull__(2)));