From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: guard against missing qs->file
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 30 Jan 2024 10:49:47 +0100

On 2024/01/29 14:59:45 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Jan 29, 2024 at 02:52:04PM +0100, Omar Polo wrote:
> > I can remove the qs->folder check if you prefer.
> 
> Please remove it from the other place instead.
> 
> Overall, I would prefer to keep input validation checks in one place.
> That makes it easier to spot problems where such checks are missing.
> 
> So when adding the qs->folder check at this higher level please remove it
> from the lower level so that we converge towards eventual consistency.

sorry for the delay, here's the updated diff.

diff /home/op/w/got
commit - d4fbd6eb2ce77846055692cfe05c18e8fabe2dca
path + /home/op/w/got
blob - 09c4ddba2490572fbc8bbc7dd65fcc63ca5eb5bf
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -1005,8 +1005,7 @@ got_output_file_blame(struct request *c, got_render_bl
 	bca.lines = NULL;
 	bca.cb = cb;
 
-	if (asprintf(&path, "%s%s%s", qs->folder ? qs->folder : "",
-	    qs->folder ? "/" : "", qs->file) == -1) {
+	if (asprintf(&path, "%s/%s", qs->folder, qs->file) == -1) {
 		error = got_error_from_errno("asprintf");
 		goto done;
 	}
blob - 62d7787e1646b5e3c0bf02940f4da590182f7c29
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -206,6 +206,11 @@ gotweb_process_request(struct request *c)
 	}
 
 	if (qs->action == BLOBRAW || qs->action == BLOB) {
+		if (qs->folder == NULL || qs->file == NULL) {
+			error = got_error(GOT_ERR_BAD_QUERYSTRING);
+			goto err;
+		}
+
 		error = got_get_repo_commits(c, 1);
 		if (error)
 			goto err;
@@ -218,6 +223,10 @@ gotweb_process_request(struct request *c)
 
 	switch (qs->action) {
 	case BLAME:
+		if (qs->folder == NULL || qs->file == NULL) {
+			error = got_error(GOT_ERR_BAD_QUERYSTRING);
+			goto err;
+		}
 		error = got_get_repo_commits(c, 1);
 		if (error) {
 			log_warnx("%s: %s", __func__, error->msg);