"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotwebd: reply with non-200 HTTP status code
To:
gameoftrees@openbsd.org
Date:
Sat, 11 Mar 2023 12:16:30 +0100

Download raw body.

Thread
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, "<div id='err_content'>%s</div>\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, "<div id='err_content'>") == -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, "</div>\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 *
 </html>
 {{ end }}
 
+{{ define gotweb_render_error(struct template *tp) }}
+{!
+	struct request		*c = tp->tp_arg;
+	struct transport	*t = c->t;
+!}
+<div id="err_content">
+  {{ if t->error }}
+    {{ t->error->msg }}
+  {{ else }}
+    See daemon logs for details
+  {{ end }}
+</div>
+{{ end }}
+
 {{ define gotweb_render_repo_table_hdr(struct template *tp) }}
 {!
 	struct request *c = tp->tp_arg;