From: Mark Jamsek Subject: Re: gotwebd: refactor gotweb_render_content_type/_file To: Omar Polo Cc: gameoftrees@openbsd.org Date: Sun, 15 Jan 2023 22:16:31 +1100 On 23-01-15 09:45AM, Omar Polo wrote: > Hello, > > Now that everything^1 runs under the templates, I'd like to refactor > gotweb_process_request so that we return a non-200 code on failure. > The first step however, is to have a function that allow us to set a > custom response code: this is done with the Status header on FastCGI. > > Diff below does a bit of cleanup, renames the functions (opinable; but > I liked a shorter name, not attached to it however, will keep the old > name if it's preferred) and allows to set both Location and Status. > > What do you think? I like the move. I'm also partial to shorter naming provided it doesn't compromise semantics, and in this case, I don't think it does so it looks good to me. I'd say ok but it would be good to hear from tracey too. > It's the first diff in a while that doesn't remove more lines than it > adds, it's a tie 54-54 this time. > > ^1: almost everything, the error page is still rendered by hand, will > get there... > > > ----------------------------------------------- > commit a5155be00a47787bc9b3fcbfc41eb761bf03559e (main) > from: Omar Polo > date: Sun Jan 15 08:43:58 2023 UTC > > gotwebd: refactor gotweb_render_content_type/_file > > Rework them so that they allow to set the Status header (the HTTP status > code; only way since we're behind FastCGI) and optionally a Location. > > Since they're now unused outside of gotweb.c, mark them as static. They > also used to always return NULL so the error is pointless; return the -1 > on failure though. > > While here, rename to gotweb_reply and gotweb_reply_file. > > diff 760079985fc2d63ebd4155a76d4f0d20fbc2f4c5 a5155be00a47787bc9b3fcbfc41eb761bf03559e > commit - 760079985fc2d63ebd4155a76d4f0d20fbc2f4c5 > commit + a5155be00a47787bc9b3fcbfc41eb761bf03559e > blob - f130435179a5b33ae9f100337a219e97ce5607aa > blob + a7105e53d7ad053fa0fcdd9dcfa7c202f39ec3fb > --- gotwebd/gotweb.c > +++ gotwebd/gotweb.c > @@ -99,6 +99,46 @@ void > > struct server *gotweb_get_server(uint8_t *, uint8_t *); > > +static int > +gotweb_reply(struct request *c, int status, const char *ctype, > + struct gotweb_url *location) > +{ > + const char *csp; > + > + if (status != 200 && fcgi_printf(c, "Status: %d\r\n", status) == -1) > + return -1; > + > + if (location) { > + if (fcgi_puts(c->tp, "Location: ") == -1 || > + gotweb_render_url(c, location) == -1 || > + fcgi_puts(c->tp, "\r\n") == -1) > + return -1; > + } > + > + csp = "Content-Security-Policy: default-src 'self'; " > + "script-src 'none'; object-src 'none';\r\n"; > + if (fcgi_puts(c->tp, csp) == -1) > + return -1; > + > + if (ctype && fcgi_printf(c, "Content-Type: %s\r\n", ctype) == -1) > + return -1; > + > + return fcgi_puts(c->tp, "\r\n"); > +} > + > +static int > +gotweb_reply_file(struct request *c, const char *ctype, const char *file, > + const char *suffix) > +{ > + int r; > + > + r = fcgi_printf(c, "Content-Disposition: attachment; " > + "filename=%s%s\r\n", file, suffix ? suffix : ""); > + if (r == -1) > + return -1; > + return gotweb_reply(c, 200, ctype, NULL); > +} > + > void > gotweb_process_request(struct request *c) > { > @@ -170,7 +210,7 @@ gotweb_process_request(struct request *c) > if (qs->action == BLOBRAW) { > const uint8_t *buf; > size_t len; > - int binary; > + int binary, r; > > error = got_get_repo_commits(c, 1); > if (error) > @@ -181,15 +221,12 @@ gotweb_process_request(struct request *c) > goto render; > > if (binary) > - error = gotweb_render_content_type_file(c, > - "application/octet-stream", qs->file, NULL); > + r = gotweb_reply_file(c, "application/octet-stream", > + qs->file, NULL); > else > - error = gotweb_render_content_type(c, "text/plain"); > - > - if (error) { > - log_warnx("%s: %s", __func__, error->msg); > + r = gotweb_reply(c, 200, "text/plain", NULL); > + if (r == -1) > goto done; > - } > > for (;;) { > error = got_object_blob_read_block(&len, blob); > @@ -225,23 +262,17 @@ gotweb_process_request(struct request *c) > if (error2) > goto render; > if (binary) { > - fcgi_puts(c->tp, "Status: 302\r\n"); > - fcgi_puts(c->tp, "Location: "); > - gotweb_render_url(c, &url); > - fcgi_puts(c->tp, "\r\n\r\n"); > + gotweb_reply(c, 302, NULL, &url); > goto done; > } > } > > if (qs->action == RSS) { > - error = gotweb_render_content_type_file(c, > - "application/rss+xml;charset=utf-8", > - repo_dir->name, ".rss"); > - if (error) { > - log_warnx("%s: %s", __func__, error->msg); > - goto err; > - } > + const char *ctype = "application/rss+xml;charset=utf-8"; > > + if (gotweb_reply_file(c, ctype, repo_dir->name, ".rss") == -1) > + goto done; > + > error = got_get_repo_tags(c, D_MAXSLCOMMDISP); > if (error) { > log_warnx("%s: %s", __func__, error->msg); > @@ -253,11 +284,8 @@ render: > } > > render: > - error = gotweb_render_content_type(c, "text/html"); > - if (error) { > - log_warnx("%s: %s", __func__, error->msg); > - goto err; > - } > + if (gotweb_reply(c, 200, "text/html", NULL) == -1) > + goto done; > html = 1; > > if (gotweb_render_header(c->tp) == -1) > @@ -760,29 +788,6 @@ const struct got_error * > free(t); > } > > -const struct got_error * > -gotweb_render_content_type(struct request *c, const char *type) > -{ > - const char *csp = "default-src 'self'; script-src 'none'; " > - "object-src 'none';"; > - > - fcgi_printf(c, > - "Content-Security-Policy: %s\r\n" > - "Content-Type: %s\r\n\r\n", > - csp, type); > - return NULL; > -} > - > -const struct got_error * > -gotweb_render_content_type_file(struct request *c, const char *type, > - const char *file, const char *suffix) > -{ > - fcgi_printf(c, "Content-type: %s\r\n" > - "Content-disposition: attachment; filename=%s%s\r\n\r\n", > - type, file, suffix ? suffix : ""); > - return NULL; > -} > - > void > gotweb_get_navs(struct request *c, struct gotweb_url *prev, int *have_prev, > struct gotweb_url *next, int *have_next) > blob - bc8d26743466b3016093da2bab2a199948bf752a > blob + 66a28ad4b968ae378a24ea55deb2ea6001240067 > --- gotwebd/gotwebd.h > +++ gotwebd/gotwebd.h > @@ -449,11 +449,6 @@ const struct got_error *gotweb_render_content_type(str > int sockets_privinit(struct gotwebd *, struct socket *); > > /* gotweb.c */ > -const struct got_error *gotweb_render_content_type(struct request *, > - const char *); > -const struct got_error > - *gotweb_render_content_type_file(struct request *, const char *, > - const char *, const char *); > void gotweb_get_navs(struct request *, struct gotweb_url *, int *, > struct gotweb_url *, int *); > const struct got_error *gotweb_get_time_str(char **, time_t, int); > -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68