From: Omar Polo Subject: gotwebd: reply with non-200 HTTP status code To: gameoftrees@openbsd.org Date: Sat, 11 Mar 2023 12:16:30 +0100 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? 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;