Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> [...]
> I've already sent this diff to kirill@, and here is the rebase following
> the latest commits. This diff fix small typos/minor issues and impl. the
> error path:

haven't had the time yet to properly test it, and sorry if i'm joining
the thread so late, but fwiw I wanted this for quite a long time, thanks
for working on it :)

it's overall okay op@, i'm just leaving some nits below, but nothing
major.

> [...]
> diff --git a/parse.y b/parse.y
> index 65664e1..4b967ca 100644
> --- a/parse.y
> +++ b/parse.y
> [...]
> +optalways	:
> +		/* empty */ { $$ = 0; }
> +		| ALWAYS    { $$ = 1; }
> +		;
> +
> +header		: HEADER REMOVE STRING optalways	{
> +			struct custom_header	*hdr;
> +
> +			if (strlen($3) > HTTPD_HEADER_NAME_MAX - 1) {
> +				yyerror("header name too long (max %d)",
> +				    HTTPD_HEADER_NAME_MAX - 1);
> +				free($3);
> +				YYERROR;
> +			}
> +
> +			if (header_name_forbidden($3)) {
> +				free($3);
> +				YYERROR;
> +			}
> +
> +			if ((hdr = calloc(1, sizeof(*hdr))) == NULL)
> +				fatal("out of memory");
> +
> +			if ((hdr->name = strdup($3)) == NULL ||
> +			    (hdr->value = strdup("")) == NULL)	/* never NULL */
> +				fatal("out of memory");
> +			free($3);

why not just

+			hdr->name = $3;

instead of of dup'ing plus freeing it?  same for the hunks below, where
we could also have `hdr->value = $4'.

> +			hdr->flags = HEADER_REMOVE;
> +			if ($4)
> +				hdr->flags |= HEADER_ALWAYS;
> +			TAILQ_INSERT_TAIL(&srv->srv_conf.headers, hdr, entry);
> +		}
> +		| HEADER ADD STRING STRING optalways	{
> +			struct custom_header	*hdr;
> +
> +			if (strlen($3) > HTTPD_HEADER_NAME_MAX - 1) {
> +				yyerror("header name too long (max %d)",
> +				    HTTPD_HEADER_NAME_MAX - 1);
> +				free($3);
> +				free($4);
> +				YYERROR;
> +			}
> +			if (header_name_forbidden($3)) {
> +				free($3);
> +				free($4);
> +				YYERROR;
> +			}
> +			if (strlen($4) > HTTPD_HEADER_VAL_MAX - 1) {
> +				yyerror("header value too long (max %d)",
> +				    HTTPD_HEADER_VAL_MAX - 1);
> +				free($3);
> +				free($4);
> +				YYERROR;
> +			}
> +
> +			if ((hdr = calloc(1, sizeof(*hdr))) == NULL)
> +				fatal("out of memory");
> +
> +			if ((hdr->name = strdup($3)) == NULL ||
> +			    (hdr->value = strdup($4)) == NULL)
> +				fatal("out of memory");
> +
> +			free($3);
> +			free($4);

i meant here (and in the next block as well) where it could have been just

			hdr->name = $3;
			hdr->value = $4;

nad move on.

> +			hdr->flags = HEADER_ADD;
> +			if ($5)
> +				hdr->flags |= HEADER_ALWAYS;
> +			TAILQ_INSERT_TAIL(&srv->srv_conf.headers, hdr, entry);
> [...]
> --- a/server_http.c
> +++ b/server_http.c
> [...]
> @@ -975,8 +985,8 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  
>  	if (srv_conf->flags & SRVFLAG_SERVER_HSTS &&
>  	    srv_conf->flags & SRVFLAG_TLS) {
> -		if (asprintf(&hstsheader, "Strict-Transport-Security: "
> -		    "max-age=%d%s%s\r\n", srv_conf->hsts_max_age,
> +		if (asprintf(&hstsheader, "max-age=%d%s%s",
> +		    srv_conf->hsts_max_age,
>  		    srv_conf->hsts_flags & HSTSFLAG_SUBDOMAINS ?
>  		    "; includeSubDomains" : "",
>  		    srv_conf->hsts_flags & HSTSFLAG_PRELOAD ?
> @@ -984,45 +994,52 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  			hstsheader = NULL;
>  			goto done;

since we only use hstsheader in this if block, why not moving the
variable locally in this block?  doing this free() + hstsheader = NULL
dance when in `goto done' we don't touch it anymore is a bit weird.

>  		}
> +
> +		ret = kv_add(&http_headers, "Strict-Transport-Security",
> +		    hstsheader) == NULL;
> +		free(hstsheader);
> +		hstsheader = NULL;
> +		if (ret)
> +			goto done;
>  	}
> +	has_body = !((code >= 100 && code < 200) || code == 204);
> +	if (has_body) {
> +		ret = snprintf(hbodylen, sizeof(hbodylen), "%zd", bodylen);
> +		if (ret < 0 || (size_t)ret >= sizeof(hbodylen))
> +			goto done;
>  
> [...]
> +int
> +server_custom_headers(struct server_config *srv_conf, struct kvtree *headers,
> +    unsigned int code)
> +{
> +	struct custom_header	*hdr;
> +	struct kv		*kv, search;
> +
> +	TAILQ_FOREACH(hdr, &srv_conf->headers, entry) {
> +		/* Only include headers not marked ALWAYS on success. */

i've found this comment a bit hard to parse.  maybe "only include
headers marked as ALWAYS on failure"?

> +		if (!(hdr->flags & HEADER_ALWAYS) && !http_is_success(code)) {
> +			print_custom_header("skip", hdr);
> +			continue;
> +		}
> +
> +		search.kv_key = hdr->name;
> +
> +		/* deletes all existing headers of the same key */
> +		if (hdr->flags & HEADER_REMOVE) {
> +			print_custom_header("remove", hdr);
> +			while ((kv = kv_find(headers, &search)) != NULL)
> +				kv_delete(headers, kv);
> +		/* replaces all existing headers of the same name */
> +		} else if (hdr->flags & HEADER_SET) {
> +			print_custom_header("set", hdr);
> +			while ((kv = kv_find(headers, &search)) != NULL)
> +				kv_delete(headers, kv);
> +
> +			if (kv_add(headers, hdr->name, hdr->value) == NULL)
> +				return (-1);
> +		/* appends a new header without checking for duplicates */
> +		} else if (hdr->flags & HEADER_ADD) {
> +			print_custom_header("add", hdr);
> +			if (kv_add(headers, hdr->name, hdr->value) == NULL)
> +				return (-1);
> +		}
> +	}
> +	return (0);
> +}