gotwebd: some html fixes

On Fri, Aug 19, 2022 at 07:30:59PM +0200, Omar Polo wrote: > as all the tragic stories goes, it all started with a simple goal of > making the W3C validator happy and endend up in pain and (html) > escaping. > > The w3c validator complained about three things: > > - invalid target=_sotd in a link > - unexpected `alt' attribute on some links > - missing lang on the html tag. > > the last one is questionable: we don't really know in what language > the commits will be (especially when they don't need to be in the same > language!) but still i think that since the whole UI is in english > there's not much harm in setting lang=en. > > Then, I saw a few places where we forgot to escape some strings and > re-cheked all the fcgi_printf calls. > > So, I'd like to propose diff below. it's actually two commit > together, I'm bundling them here because it's easier. It fixes the > issue reported by the w3c validator and escapes all the potential > unsafe strings I've found. > > There's still a bit that's missing and it's properly URL-escape links, > which is different from the html escaping, but i'd like to address > that in a follow up commit. gotwebd needs to learn how to > percent-decode querystrings too. > > I have this diff on my instance, now I can render 'funny' trees with > files like '<script>' :) > > https://git.omarpolo.com/?index_page=0&path=testing.git&action=tree&commit=78397ac198dbef68c371cef13a4fb986ff93fe34&headref=HEAD > > (note that the first two links don't work because of the escaping) > > thoughts/ok? Ok, although I'm not sold on the lang=en. > > diff refs/remotes/got/main refs/heads/main > commit - 01498c42e0fc9fb6355312a236656cf2f36cebc0 > commit + 0e9860257eadd668942affe9f2846a6e09603e3b > blob - 35735937cc54916d0fa7248397a0a66248c0c510 > blob + 18dca123b78589b4efb8482bc4caa54e1aae7176 > --- gotwebd/got_operations.c > +++ gotwebd/got_operations.c > @@ -830,7 +830,7 @@ got_output_repo_tree(struct request *c) > struct got_tree_object *tree = NULL; > struct repo_dir *repo_dir = t->repo_dir; > const char *name, *index_page_str, *folder; > - char *id_str = NULL; > + char *id_str = NULL, *escaped_name; > char *path = NULL, *in_repo_path = NULL, *modestr = NULL; > int nentries, i, r; > > @@ -920,8 +920,12 @@ got_output_repo_tree(struct request *c) > } > } > > + name = got_tree_entry_get_name(te); > + error = gotweb_escape_html(&escaped_name, name); > + if (error) > + goto done; > + > if (S_ISDIR(mode)) { > - name = got_tree_entry_get_name(te); > r = fcgi_printf(c, > "<div class='tree_wrapper'>\n" > "<div class='tree_line'>" > @@ -932,11 +936,10 @@ got_output_repo_tree(struct request *c) > "<div class='tree_line_blank'> </div>\n" > "</div>\n", /* .tree_wrapper */ > index_page_str, qs->path, rc->commit_id, > - folder, name, name, modestr); > + folder, name, escaped_name, modestr); > if (r == -1) > goto done; > } else { > - name = got_tree_entry_get_name(te); > r = fcgi_printf(c, > "<div class='tree_wrapper'>\n" > "<div class='tree_line'>" > @@ -952,7 +955,7 @@ got_output_repo_tree(struct request *c) > "</div>\n" /* .tree_line_blank */ > "</div>\n", /* .tree_wrapper */ > index_page_str, qs->path, rc->commit_id, > - folder, name, name, modestr, > + folder, name, escaped_name, modestr, > index_page_str, qs->path, rc->commit_id, > folder, name, > index_page_str, qs->path, rc->commit_id, > blob - 9358e442cb79338b33110402f61e9bd695a9690d > blob + 4dfa5656c52bd7eff6cdc8b2196476bb8e664581 > --- gotwebd/gotweb.c > +++ gotwebd/gotweb.c > @@ -655,7 +655,7 @@ gotweb_render_header(struct request *c) > strlcpy(droot, "/", sizeof(droot)); > > r = fcgi_printf(c, "<!doctype html>\n" > - "<html>\n" > + "<html lang=\"en\">\n" > "<head>\n" > "<title>%s</title>\n" > "<meta charset='utf-8' />\n" > @@ -676,14 +676,14 @@ gotweb_render_header(struct request *c) > "<div id='gw_body'>\n" > "<div id='header'>\n" > "<div id='got_link'>" > - "<a href='%s' target='_sotd'>" > + "<a href='%s' target='_blank'>" > "<img src='%s%s' alt='logo' id='logo' />" > "</a>\n" > "</div>\n" /* #got_link */ > "</div>\n" /* #header */ > "<div id='site_path'>\n" > "<div id='site_link'>\n" > - "<a href='/%s?index_page=%d' alt='sitelink'>%s</a>", > + "<a href='/%s?index_page=%d'>%s</a>", > srv->site_name, > droot, srv->custom_css, > srv->logo_url, > @@ -695,8 +695,8 @@ gotweb_render_header(struct request *c) > if (qs != NULL) { > if (qs->path != NULL) { > r = fcgi_printf(c, " / " > - "<a href='/%s?index_page=%d&path=%s&action=summary'" > - " alt='summlink'>%s</a>", > + "<a href='/%s?index_page=%d&path=%s&action=summary'>" > + "%s</a>", > c->document_root, qs->index_page, qs->path, > qs->path); > if (r == -1) > @@ -1136,7 +1136,7 @@ gotweb_render_blame(struct request *c) > const struct got_error *error = NULL; > struct transport *t = c->t; > struct repo_commit *rc = NULL; > - char *age = NULL; > + char *age = NULL, *msg = NULL; > int r; > > error = got_get_repo_commits(c, 1); > @@ -1148,6 +1148,9 @@ gotweb_render_blame(struct request *c) > error = gotweb_get_time_str(&age, rc->committer_time, TM_LONG); > if (error) > goto done; > + error = gotweb_escape_html(&msg, rc->commit_msg); > + if (error) > + goto done; > > r = fcgi_printf(c, "<div id='blame_title_wrapper'>\n" > "<div id='blame_title'>Blame</div>\n" > @@ -1164,7 +1167,7 @@ gotweb_render_blame(struct request *c) > "<div class='dotted_line'></div>\n" > "<div id='blame'>\n", > age ? age : "", > - rc->commit_msg); > + msg); > if (r == -1) > goto done; > > @@ -1175,6 +1178,7 @@ gotweb_render_blame(struct request *c) > fcgi_printf(c, "</div>\n" /* #blame */ > "</div>\n"); /* #blame_content */ > done: > + free(msg); > return error; > } > > @@ -1189,7 +1193,7 @@ gotweb_render_briefs(struct request *c) > struct repo_dir *repo_dir = t->repo_dir; > const char *index_page_str; > char *smallerthan, *newline; > - char *age = NULL; > + char *age = NULL, *author = NULL, *msg = NULL; > int r; > > index_page_str = qs->index_page_str ? qs->index_page_str : ""; > @@ -1222,22 +1226,34 @@ gotweb_render_briefs(struct request *c) > if (newline) > *newline = '\0'; > > + error = gotweb_escape_html(&author, rc->author); > + if (error) > + goto done; > + error = gotweb_escape_html(&msg, rc->commit_msg); > + if (error) > + goto done; > + > 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 ? age : "", > - rc->author, > + author, > index_page_str, repo_dir->name, rc->commit_id, qs->headref, > - rc->commit_msg); > + msg); > if (r == -1) > goto done; > > if (rc->refs_str) { > + char *refs; > + > + error = gotweb_escape_html(&refs, rc->refs_str); > + if (error) > + goto done; > r = fcgi_printf(c, > - " <span class='refs_str'>(%s)</span>", > - rc->refs_str); > + " <span class='refs_str'>(%s)</span>", refs); > + free(refs); > if (r == -1) > goto done; > } > @@ -1261,6 +1277,10 @@ gotweb_render_briefs(struct request *c) > > free(age); > age = NULL; > + free(author); > + author = NULL; > + free(msg); > + msg = NULL; > } > > if (t->next_id || t->prev_id) { > @@ -1271,6 +1291,8 @@ gotweb_render_briefs(struct request *c) > fcgi_printf(c, "</div>\n"); /* #briefs_content */ > done: > free(age); > + free(author); > + free(msg); > return error; > } > > @@ -1284,7 +1306,7 @@ gotweb_render_commits(struct request *c) > struct querystring *qs = t->qs; > struct repo_dir *repo_dir = t->repo_dir; > const char *index_page_str; > - char *age = NULL, *author = NULL; > + char *age = NULL, *author = NULL, *msg = NULL; > int r; > > index_page_str = qs->index_page_str ? qs->index_page_str : ""; > @@ -1307,6 +1329,9 @@ gotweb_render_commits(struct request *c) > error = gotweb_escape_html(&author, rc->author); > if (error) > goto done; > + error = gotweb_escape_html(&msg, rc->commit_msg); > + if (error) > + goto done; > > r = fcgi_printf(c, "<div class='commits_header_wrapper'>\n" > "<div class='commits_header'>\n" > @@ -1321,9 +1346,9 @@ gotweb_render_commits(struct request *c) > "<div class='dotted_line'></div>\n" > "<div class='commit'>\n%s</div>\n", > rc->commit_id, > - author ? author : "", > + author, > age ? age : "", > - rc->commit_msg); > + msg); > if (r == -1) > goto done; > > @@ -1344,6 +1369,8 @@ gotweb_render_commits(struct request *c) > age = NULL; > free(author); > author = NULL; > + free(msg); > + msg = NULL; > } > > if (t->next_id || t->prev_id) { > @@ -1354,6 +1381,8 @@ gotweb_render_commits(struct request *c) > fcgi_printf(c, "</div>\n"); /* .commits_content */ > done: > free(age); > + free(author); > + free(msg); > return error; > } > > @@ -1387,12 +1416,13 @@ gotweb_render_branches(struct request *c) > goto done; > > TAILQ_FOREACH(re, &refs, entry) { > - char *refname = NULL; > + const char *refname = NULL; > + char *escaped_refname = NULL; > > if (got_ref_is_symbolic(re->ref)) > continue; > > - refname = strdup(got_ref_get_name(re->ref)); > + refname = got_ref_get_name(re->ref); > if (refname == NULL) { > error = got_error_from_errno("strdup"); > goto done; > @@ -1407,6 +1437,9 @@ gotweb_render_branches(struct request *c) > > if (strncmp(refname, "refs/heads/", 11) == 0) > refname += 11; > + error = gotweb_escape_html(&escaped_refname, refname); > + if (error) > + goto done; > > r = fcgi_printf(c, "<div class='branches_wrapper'>\n" > "<div class='branches_age'>%s</div>\n" > @@ -1431,10 +1464,11 @@ gotweb_render_branches(struct request *c) > "</div>\n", /* .branches_wrapper */ > age ? age : "", > index_page_str, qs->path, refname, > - refname, > + escaped_refname, > index_page_str, qs->path, refname, > index_page_str, qs->path, refname, > index_page_str, qs->path, refname); > + free(escaped_refname); > if (r == -1) > goto done; > > @@ -1453,7 +1487,7 @@ gotweb_render_tree(struct request *c) > const struct got_error *error = NULL; > struct transport *t = c->t; > struct repo_commit *rc = NULL; > - char *age = NULL; > + char *age = NULL, *msg = NULL; > int r; > > error = got_get_repo_commits(c, 1); > @@ -1466,6 +1500,10 @@ gotweb_render_tree(struct request *c) > if (error) > goto done; > > + error = gotweb_escape_html(&msg, rc->commit_msg); > + if (error) > + goto done; > + > r = fcgi_printf(c, "<div id='tree_title_wrapper'>\n" > "<div id='tree_title'>Tree</div>\n" > "</div>\n" /* #tree_title_wrapper */ > @@ -1484,7 +1522,7 @@ gotweb_render_tree(struct request *c) > "<div id='tree'>\n", > rc->tree_id, > age ? age : "", > - rc->commit_msg); > + msg); > if (r == -1) > goto done; > > @@ -1495,6 +1533,7 @@ gotweb_render_tree(struct request *c) > fcgi_printf(c, "</div>\n"); /* #tree */ > fcgi_printf(c, "</div>\n"); /* #tree_content */ > done: > + free(msg); > return error; > } > > @@ -1504,7 +1543,7 @@ gotweb_render_diff(struct request *c) > const struct got_error *error = NULL; > struct transport *t = c->t; > struct repo_commit *rc = NULL; > - char *age = NULL, *author = NULL; > + char *age = NULL, *author = NULL, *msg = NULL; > int r; > > error = got_get_repo_commits(c, 1); > @@ -1519,6 +1558,9 @@ gotweb_render_diff(struct request *c) > error = gotweb_escape_html(&author, rc->author); > if (error) > goto done; > + error = gotweb_escape_html(&msg, rc->commit_msg); > + if (error) > + goto done; > > r = fcgi_printf(c, "<div id='diff_title_wrapper'>\n" > "<div id='diff_title'>Commit Diff</div>\n" > @@ -1545,9 +1587,9 @@ gotweb_render_diff(struct request *c) > rc->parent_id, rc->commit_id, > rc->commit_id, > rc->tree_id, > - author ? author : "", > + author, > age ? age : "", > - rc->commit_msg); > + msg); > if (r == -1) > goto done; > > @@ -1560,6 +1602,7 @@ gotweb_render_diff(struct request *c) > done: > free(age); > free(author); > + free(msg); > return error; > } > > @@ -1578,7 +1621,7 @@ gotweb_render_summary(struct request *c) > r = fcgi_printf(c, > "<div id='description_title'>Description:</div>\n" > "<div id='description'>%s</div>\n", > - t->repo_dir->description); > + t->repo_dir->description ? t->repo_dir->description : ""); > if (r == -1) > goto done; > } > @@ -1587,7 +1630,7 @@ gotweb_render_summary(struct request *c) > r = fcgi_printf(c, > "<div id='repo_owner_title'>Owner:</div>\n" > "<div id='repo_owner'>%s</div>\n", > - t->repo_dir->owner); > + t->repo_dir->owner ? t->repo_dir->owner : ""); > if (r == -1) > goto done; > } > @@ -1639,7 +1682,7 @@ gotweb_render_tag(struct request *c) > const struct got_error *error = NULL; > struct repo_tag *rt = NULL; > struct transport *t = c->t; > - char *age = NULL, *author = NULL; > + char *tagname = NULL, *age = NULL, *author = NULL, *msg = NULL; > > error = got_get_repo_tags(c, 1); > if (error) > @@ -1659,9 +1702,15 @@ gotweb_render_tag(struct request *c) > error = gotweb_escape_html(&author, rt->tagger); > if (error) > goto done; > + error = gotweb_escape_html(&msg, rt->commit_msg); > + if (error) > + goto done; > > if (strncmp(rt->tag_name, "refs/", 5) == 0) > rt->tag_name += 5; > + error = gotweb_escape_html(&tagname, rt->tag_name); > + if (error) > + goto done; > > fcgi_printf(c, "<div id='tags_title_wrapper'>\n" > "<div id='tags_title'>Tag</div>\n" > @@ -1683,15 +1732,16 @@ gotweb_render_tag(struct request *c) > "<div id='tag_commit'>\n%s</div>" > "</div>", /* tag_header_wrapper */ > rt->commit_id, > - rt->tag_name, > - author ? author : "", > + tagname, > + author, > age ? age : "", > - rt->commit_msg, > + msg, > rt->tag_commit); > > done: > free(age); > free(author); > + free(msg); > return error; > } > > @@ -1705,8 +1755,7 @@ gotweb_render_tags(struct request *c) > struct querystring *qs = t->qs; > struct repo_dir *repo_dir = t->repo_dir; > const char *index_page_str; > - char *newline; > - char *age = NULL; > + char *age = NULL, *tagname = NULL, *msg = NULL, *newline; > int r, commit_found = 0; > > index_page_str = qs->index_page_str ? qs->index_page_str : ""; > @@ -1746,11 +1795,17 @@ gotweb_render_tags(struct request *c) > > if (strncmp(rt->tag_name, "refs/tags/", 10) == 0) > rt->tag_name += 10; > + error = gotweb_escape_html(&tagname, rt->tag_name); > + if (error) > + goto done; > > if (rt->tag_commit != NULL) { > newline = strchr(rt->tag_commit, '\n'); > if (newline) > *newline = '\0'; > + error = gotweb_escape_html(&msg, rt->tag_commit); > + if (error) > + goto done; > } > > r = fcgi_printf(c, "<div class='tag_age'>%s</div>\n" > @@ -1773,9 +1828,9 @@ gotweb_render_tags(struct request *c) > "</div>\n" /* .navs_wrapper */ > "<div class='dotted_line'></div>\n", > age ? age : "", > - rt->tag_name, > + tagname, > index_page_str, repo_dir->name, rt->commit_id, > - rt->tag_commit ? rt->tag_commit : "", > + 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); > @@ -1784,6 +1839,10 @@ gotweb_render_tags(struct request *c) > > free(age); > age = NULL; > + free(tagname); > + tagname = NULL; > + free(msg); > + msg = NULL; > } > if (t->next_id || t->prev_id) { > error = gotweb_render_navs(c); > @@ -1793,6 +1852,8 @@ gotweb_render_tags(struct request *c) > fcgi_printf(c, "</div>\n"); /* #tags_content */ > done: > free(age); > + free(tagname); > + free(msg); > return error; > } > > > > -- Tracey Emery

2022-08-19 17:30 Omar Polo:
gotwebd: some html fixes
- 2022-08-19 17:36 Omar Polo:
  gotwebd: some html fixes
- 2022-08-19 18:54 Tracey Emery:
  gotwebd: some html fixes
- - 2022-08-20 13:59 Omar Polo:
    gotwebd: some html fixes