From: Tracey Emery Subject: Re: gotwebd: reply with non-200 HTTP status code To: gameoftrees@openbsd.org, Omar Polo Date: Sat, 11 Mar 2023 07:55:29 -0700 On March 11, 2023 4:16:30 AM MST, Omar Polo wrote: >Here's the second half of the previous diff; this makes gotwebd return >400 when it encounters an error processing the request. This will >make scripts, bots, crawlers and whatnot aware that the "gotwebd >experienced an error" page is, in fact, an error page and maybe stop >issuing requests for that URL. > >It also cleans the `goto render'/`goto done'/`goto err' madness: only >the `err' label remains and it's used to render the error page. (Note >that some `goto' were wrong before.) > >If an error occurs after gotweb_reply() was called, we can't do >anything about it and just `return'. All the allocations are done on >the struct transport that is free'd when tearing down the connection, >so no risk of leaking resources. (this because we can only signal an >error *before* sending the body of the page.) > >Furthermore, since we don't need to track whether we've replied or >not, the error page can be consistently be in HTML and so I've moved >it to a template while here. > >regarding the HTTP error: there are two kinds of errors > > - 4XX: the client request is "bad" (the path does not exists, wrong > querystring, ...) > > - 5XX: an internal server error handling the request (ENOMEM and > other run-time failures.) > >It's not clear to me how map a `struct got_error' to an HTTP error, so >I've picked a generic 400 "Bad Request" and went with it. Should be >"good enough"™ in practice. > >As usual, I've updated my instance with this. You can tell, for >example, because now: > > % curl --head 'https://got.omarpolo.com/?action=summary&path=got.git' > HTTP/1.1 200 OK > [...] > >while a non-existant foo.git yields > > % curl --head 'https://got.omarpolo.com/?action=summary&path=foo.git' > HTTP/1.1 400 Bad Request > [...] > >ok? I've only read this, so I'll trust you've tested. Ok > >diffstat /home/op/w/got > M gotwebd/gotweb.c | 65+ 84- > M gotwebd/gotwebd.h | 2+ 0- > M gotwebd/pages.tmpl | 14+ 0- > >3 files changed, 81 insertions(+), 84 deletions(-) > >diff /home/op/w/got >commit - df2d3cd2545e0a1579ce83ae137e52135755ed1f >path + /home/op/w/got >blob - ca03025fe2d5b211ca540d4ee3b9410d8c0ea611 >file + gotwebd/gotweb.c >--- gotwebd/gotweb.c >+++ gotwebd/gotweb.c >@@ -143,12 +143,10 @@ gotweb_process_request(struct request *c) > void > gotweb_process_request(struct request *c) > { >- const struct got_error *error = NULL, *error2 = NULL; >+ const struct got_error *error = NULL; > struct server *srv = NULL; > struct querystring *qs = NULL; > struct repo_dir *repo_dir = NULL; >- uint8_t err[] = "gotwebd experienced an error: "; >- int r, html = 0; > > /* init the transport */ > error = gotweb_init_transport(&c->t); >@@ -188,15 +186,15 @@ gotweb_process_request(struct request *c) > if (qs->action == BLAME || qs->action == BLOB || > qs->action == BLOBRAW || qs->action == DIFF) { > if (qs->commit == NULL) { >- error2 = got_error(GOT_ERR_QUERYSTRING); >- goto render; >+ error = got_error(GOT_ERR_QUERYSTRING); >+ goto err; > } > } > > if (qs->action != INDEX) { > error = gotweb_init_repo_dir(&repo_dir, qs->path); > if (error) >- goto done; >+ goto err; > error = gotweb_load_got_path(c, repo_dir); > c->t->repo_dir = repo_dir; > if (error && error->code != GOT_ERR_LONELY_PACKIDX) >@@ -210,12 +208,12 @@ gotweb_process_request(struct request *c) > > error = got_get_repo_commits(c, 1); > if (error) >- goto done; >+ goto err; > >- error2 = got_open_blob_for_output(&c->t->blob, &c->t->fd, >+ error = got_open_blob_for_output(&c->t->blob, &c->t->fd, > &binary, c); >- if (error2) >- goto render; >+ if (error) >+ goto err; > > if (binary) > r = gotweb_reply_file(c, "application/octet-stream", >@@ -223,20 +221,20 @@ gotweb_process_request(struct request *c) > else > r = gotweb_reply(c, 200, "text/plain", NULL); > if (r == -1) >- goto done; >+ return; > > for (;;) { > error = got_object_blob_read_block(&len, c->t->blob); > if (error) >- goto done; >+ break; > if (len == 0) > break; > buf = got_object_blob_get_read_buf(c->t->blob); > if (fcgi_gen_binary_response(c, buf, len) == -1) >- goto done; >+ break; > } > >- goto done; >+ return; > } > > if (qs->action == BLOB) { >@@ -253,15 +251,15 @@ gotweb_process_request(struct request *c) > > error = got_get_repo_commits(c, 1); > if (error) >- goto done; >+ goto err; > >- error2 = got_open_blob_for_output(&c->t->blob, &c->t->fd, >+ error = got_open_blob_for_output(&c->t->blob, &c->t->fd, > &binary, c); >- if (error2) >- goto render; >+ if (error) >+ goto err; > if (binary) { > gotweb_reply(c, 302, NULL, &url); >- goto done; >+ return; > } > } > >@@ -269,28 +267,17 @@ gotweb_process_request(struct request *c) > const char *ctype = "application/rss+xml;charset=utf-8"; > > if (gotweb_reply_file(c, ctype, repo_dir->name, ".rss") == -1) >- goto done; >+ return; > > error = got_get_repo_tags(c, D_MAXSLCOMMDISP); > if (error) { > log_warnx("%s: %s", __func__, error->msg); >- goto err; >+ return; > } >- if (gotweb_render_rss(c->tp) == -1) >- goto err; >- goto done; >+ gotweb_render_rss(c->tp); >+ return; > } > >-render: >- if (gotweb_reply(c, 200, "text/html", NULL) == -1) >- goto done; >- html = 1; >- >- if (error2) { >- error = error2; >- goto err; >- } >- > switch(qs->action) { > case BLAME: > error = got_get_repo_commits(c, 1); >@@ -298,26 +285,30 @@ render: > log_warnx("%s: %s", __func__, error->msg); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_blame) == -1) >- goto done; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_blame); >+ return; > case BLOB: >- if (gotweb_render_page(c->tp, gotweb_render_blob) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_blob); >+ return; > case BRIEFS: >- if (gotweb_render_page(c->tp, gotweb_render_briefs) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_briefs); >+ return; > case COMMITS: > error = got_get_repo_commits(c, srv->max_commits_display); > if (error) { > log_warnx("%s: %s", __func__, error->msg); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_commits) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_commits); >+ return; > case DIFF: > error = got_get_repo_commits(c, 1); > if (error) { >@@ -329,9 +320,10 @@ render: > log_warnx("%s: %s", __func__, error->msg); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_diff) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_diff); >+ return; > case INDEX: > c->t->nrepos = scandir(srv->repos_path, &c->t->repos, NULL, > alphasort); >@@ -341,9 +333,10 @@ render: > srv->repos_path); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_index) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_index); >+ return; > case SUMMARY: > error = got_ref_list(&c->t->refs, c->t->repo, "refs/heads", > got_ref_cmp_by_name, NULL); >@@ -360,9 +353,10 @@ render: > goto err; > } > qs->action = SUMMARY; >- if (gotweb_render_page(c->tp, gotweb_render_summary) == -1) >- goto done; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_summary); >+ return; > case TAG: > error = got_get_repo_tags(c, 1); > if (error) { >@@ -374,53 +368,40 @@ render: > "bad commit id"); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_tag) == -1) >- goto done; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_tag); >+ return; > case TAGS: > error = got_get_repo_tags(c, srv->max_commits_display); > if (error) { > log_warnx("%s: %s", __func__, error->msg); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_tags) == -1) >- goto done; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_tags); >+ return; > case TREE: > error = got_get_repo_commits(c, 1); > if (error) { > log_warnx("%s: %s", __func__, error->msg); > goto err; > } >- if (gotweb_render_page(c->tp, gotweb_render_tree) == -1) >- goto err; >- break; >+ if (gotweb_reply(c, 200, "text/html", NULL) == -1) >+ return; >+ gotweb_render_page(c->tp, gotweb_render_tree); >+ return; > case ERR: > default: >- r = fcgi_printf(c, "
%s
\n", >- "Erorr: Bad Querystring"); >- if (r == -1) >- goto err; >- break; >+ error = got_error(GOT_ERR_BAD_QUERYSTRING); > } > >- goto done; > err: >- if (html && fcgi_printf(c, "
") == -1) >+ c->t->error = error; >+ if (gotweb_reply(c, 400, "text/html", NULL) == -1) > return; >- if (fcgi_printf(c, "\n%s", err) == -1) >- return; >- if (error) { >- if (fcgi_printf(c, "%s", error->msg) == -1) >- return; >- } else { >- if (fcgi_printf(c, "see daemon logs for details") == -1) >- return; >- } >- if (html && fcgi_printf(c, "
\n") == -1) >- return; >-done: >- return; >+ gotweb_render_page(c->tp, gotweb_render_error); > } > > struct server * >blob - 56c7eb962abf8757106cc2b4836d502c6a6655e9 >file + gotwebd/gotwebd.h >--- gotwebd/gotwebd.h >+++ gotwebd/gotwebd.h >@@ -197,6 +197,7 @@ struct transport { > unsigned int next_disp; > unsigned int prev_disp; > unsigned int tag_count; >+ const struct got_error *error; > struct got_blob_object *blob; > int fd; > FILE *fp; >@@ -470,6 +471,7 @@ int gotweb_render_repo_table_hdr(struct template *); > > /* pages.tmpl */ > int gotweb_render_page(struct template *, int (*)(struct template *)); >+int gotweb_render_error(struct template *); > int gotweb_render_repo_table_hdr(struct template *); > int gotweb_render_repo_fragment(struct template *, struct repo_dir *); > int gotweb_render_briefs(struct template *); >blob - 6517aad5f520ea885ec366bd60717093b4ce85ae >file + gotwebd/pages.tmpl >--- gotwebd/pages.tmpl >+++ gotwebd/pages.tmpl >@@ -125,6 +125,20 @@ static inline int rss_author(struct template *, char * > > {{ end }} > >+{{ define gotweb_render_error(struct template *tp) }} >+{! >+ struct request *c = tp->tp_arg; >+ struct transport *t = c->t; >+!} >+
>+ {{ if t->error }} >+ {{ t->error->msg }} >+ {{ else }} >+ See daemon logs for details >+ {{ end }} >+
>+{{ end }} >+ > {{ define gotweb_render_repo_table_hdr(struct template *tp) }} > {! > struct request *c = tp->tp_arg; > -- Tracey Emery Sent from my phone.