On 2023/07/07 14:42:31 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> I came across this misbehaviour of gitwrapper while setting up
> a private repository to be served by gotd.
> 
> ok?

ok op@

some comments inline however.


> --- gotd/parse.y
> +++ gotd/parse.y
> @@ -312,16 +314,28 @@ repoopts1	: PATH STRING {
>  					YYERROR;
>  				}
>  				if (realpath($2, new_repo->path) == NULL) {
> -					yyerror("realpath %s: %s", $2,
> -					    strerror(errno));
>  					/*
> -					 * Give admin a chance to create
> -					 * missing repositories at run-time.
> +					 * To give admins a chance to create
> +					 * missing repositories at run-time
> +					 * we only warn about ENOENT here.

not a problem of this diff, but this comment is wrong.

	% cat >test.conf
	repository foo {
	        path '/oops.git'
	        permit rw op
	}
	% doas /usr/local/sbin/gotd -d -f ./test.conf
	gotd: ./test.conf:2: realpath /oops.git: No such file or directory
	% echo $?
	1

> +					 *
> +					 * And ignore 'permission denied' when
> +					 * running in gitwrapper. Users may be
> +					 * able to access this repository via
> +					 * gotd regardless.
>  					 */
> -					if (errno != ENOENT) {
> +					if (errno == ENOENT) {
> +						yyerror("realpath %s: %s", $2,
> +						    strerror(errno));

this however works because gitwrapper doesn't check parse_config()
return value.

(yyerror() bumps file->errors, which is then inspected by
parse_config().)

i'm starting to think that we should use log.c for these messages, and
bump file->errors if we think it's worth.  The upside would be that we
stop writing to fd 2 on error (which i'm not sure if could cause
issues when reloading the config.)

> +					} else if (errno != EACCES ||
> +					    gotd_proc_id != PROC_GITWRAPPER) {
> +						yyerror("realpath %s: %s", $2,
> +						    strerror(errno));
>  						free($2);
>  						YYERROR;
> -					} else if (strlcpy(new_repo->path, $2,
> +					}
> +
> +					if (strlcpy(new_repo->path, $2,
>  					    sizeof(new_repo->path)) >=
>  					    sizeof(new_repo->path))
>  						yyerror("path too long");
> @@ -740,10 +754,11 @@ parse_config(const char *filename, enum gotd_procid pr
>  
>  int
>  parse_config(const char *filename, enum gotd_procid proc_id,
> -    struct gotd *env, int require_config_file)
> +    struct gotd *env)
>  {
>  	struct sym *sym, *next;
>  	struct gotd_repo *repo;
> +	int require_config_file = (proc_id == PROC_GITWRAPPER ? 0 : 1);

not really an issue, but made me raise an eyebrow.  I'd rewrite it as

	int require_config_file = (proc_id != PROC_GITWRAPPER)

or something more verbose along the lines of

	int require_config_file = 1;

	if (proc_id == PROC_GITWRAPPER)
		require_config_file = 0;


your call however.