From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: percent-encode querystrings
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 6 Sep 2022 13:17:21 -0600

On Tue, Sep 06, 2022 at 05:12:14PM +0200, Omar Polo wrote:
> 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

This breaks next and previous links.

> 
> 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)));
> 

-- 

Tracey Emery