From: Tracey Emery Subject: Re: gotwebd: percent-encode querystrings To: Omar Polo , gameoftrees@openbsd.org Date: Tue, 6 Sep 2022 13:21:10 -0600 On Tue, Sep 06, 2022 at 01:17:21PM -0600, Tracey Emery wrote: > On Tue, Sep 06, 2022 at 05:12:14PM +0200, Omar Polo wrote: > > On 2022/09/03 11:27:58 +0200, Omar Polo 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. No sorry, looks like something else broke that. But no time to test right now. Look at briefs. Clicking next just now builds on the page instead of the original, correct action. > > > > > 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, > > - "
\n" > > - "\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,"
\n" > > + "
") == -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, "
\n" /* .tree_line */ > > "
 
\n" > > - "
\n", /* .tree_wrapper */ > > - index_page_str, qs->path, rc->commit_id, > > - folder, name, escaped_name, modestr); > > - if (r == -1) > > + "
\n") == -1) > > goto done; > > } else { > > - r = fcgi_printf(c, > > - "
\n" > > - "
" > > - "%s%s" > > - "
\n" /* .tree_line */ > > - "
" > > - "commits\n" > > - " | \n" > > - "blame\n" > > - "
\n" /* .tree_line_blank */ > > - "
\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, "
\n" > > + "
") == -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, "
\n" /* .tree_line */ > > + "
") == -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, > > + "
\n" /* .tree_line_blank */ > > + "
\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, "
" > > + if (fcgi_printf(c, "
" > > "
%.*d
" > > - "
" > > - "" > > - "%.8s" > > + "
", > > + 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, > > "
" > > "
%s
" > > "
%s
" > > "
%s
" > > "
", /* .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, " / " > > - "" > > - "%s", > > - 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, "
\n\n" /* #nav_prev */ > > "\n"); /* #nav_next */ > > fcgi_printf(c, "
\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, "
\n" > > - "
" > > - "" > > - " %s " > > - "" > > - "
", /* .index_project */ > > - index_page_str, repo_dir->name, > > - repo_dir->name); > > + if (fcgi_printf(c, "
\n" > > + "
") == -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, "
") == -1) /* .index_project */ > > + goto done; > > + > > if (srv->show_repo_description) { > > r = fcgi_printf(c, > > "
\n" > > @@ -1124,32 +1129,71 @@ render: > > goto done; > > } > > > > - r = fcgi_printf(c, "\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); > > + "
\n" /* .navs_wrapper */ > > + "
\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, "
\n" > > "
Commit Briefs
\n" > > "
\n" /* #briefs_title_wrapper */ > > @@ -1287,16 +1328,22 @@ gotweb_render_briefs(struct request *c) > > > > r = fcgi_printf(c, "
%s
\n" > > "
%s
\n" > > - "
" > > - "%s", > > - age, > > - author, > > - index_page_str, repo_dir->name, rc->commit_id, qs->headref, > > - msg); > > + "
", > > + 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, "\n" /* .navs_wrapper */ > > - "
\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) > > + "
\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, "
\n" > > "
Commits
\n" > > "
\n" /* .commits_title_wrapper */ > > @@ -1404,19 +1467,36 @@ gotweb_render_commits(struct request *c) > > if (r == -1) > > goto done; > > > > - r = fcgi_printf(c, "\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); > > + "
\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, "
\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, "
%s
\n" > > + if (fcgi_printf(c, "
%s
\n" > > "
%s
\n" > > - "
" > > - "" > > - "%s" > > - "
\n" /* .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, "
\n" /* .tag_log */ > > "