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

From:
Tracey Emery <tracey@traceyemery.net>
Subject:
Re: gotwebd: reply with non-200 HTTP status code
To:
gameoftrees@openbsd.org, Omar Polo <op@omarpolo.com>
Date:
Sat, 11 Mar 2023 07:55:29 -0700

Download raw body.

Thread
On March 11, 2023 4:16:30 AM MST, Omar Polo <op@omarpolo.com> 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, "<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;
>


-- 
Tracey Emery
Sent from my phone.