On Thu, Jul 16, 2026 at 07:42:36AM +0200, Rafael Sadowski wrote:
> On Wed Jul 15, 2026 at 08:56:27AM +0200, Claudio Jeker wrote:
> > On Wed, Jul 15, 2026 at 08:08:51AM +0200, Rafael Sadowski wrote:
> > > On Mon Jul 13, 2026 at 02:16:58AM +0200, Kirill A. Korinsky wrote:
> > > > On Sat, 11 Jul 2026 16:12:46 +0200,
> > > > Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> > > > > 
> > > > > I think this is a solid first version for that feature. We'll certainly
> > > > > find out about more edge cases through user feedback ... or maybe not ;)
> > > > > 
> > > > 
> > > > Here first pass for review. I haven't spent much time on this. I may be wrong.
> > > > 
> > > > > +	TAILQ_FOREACH(hdr, &parent_srv->srv_conf.headers, entry) {
> > > > > +		if (header_exists(srv_conf, hdr->name)) {
> > > > > +			DPRINTF("%s: skipping header \"%s\" from parent "
> > > > > +			    "\"%s\", overridden in location \"%s\"",
> > > > > +			    __func__, hdr->name,
> > > > > +			    parent_srv->srv_conf.name, srv_conf->location);
> > > > > +			continue;
> > > > > +		}
> > > > 
> > > > What happens with duplicated headers? Multiple Set-Cookie is good example.
> > > 
> > > Good catch!
> > > 
> > > Fixed by collecting the inherited headers in a temporary list and
> > > TAILQ_CONCAT()ing them afterwards, so config_header_exists() only ever sees
> > > headers defined in the location itself.
> > > 
> > > The semantics are unchanged otherwise: if a location mentions a header name at
> > > all, it takes over that name completely and none of the parent's headers
> > > with that name are inherited. So a "header add "Set-Cookie"" in a
> > > location replaces the whole parent group rather than adding to it. The
> > > users can use remove if they want to drop individual inherited headers.
> > > Documented in httpd.conf(5).
> > > 
> > > > 
> > > > > @@ -1050,6 +1063,7 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
> > > > >  	free(hstsheader);
> > > > >  	free(clenheader);
> > > > >  	free(bannerheader);
> > > > > +	free(customheaders);
> > > > 
> > > > I think this code is reached only on error, and it makes customheaders leaks
> > > > on sucess.
> > > > 
> > > > And next things which I not yet sure. I think that roughly 25 directives
> > > > exceed the 16K imsg limit because every one is fixed 664 byte structure nad
> > > > it is packed into one message.
> > > 
> > > So true! That's what happens when you follow the code from fastcgi.
> > > FastCGI has the same problem. It's actually even worse:
> > 
> > With imsg_set_maxsize(3) you can alter the limit. e.g. bgpd uses a 128k
> > imsg size. You can go quite large but keep in mind that the full message
> > needs to a) fit in memory and b) needs to be transferred before other
> > messages can be processed (the imsgq is a fifo).
> >  
> > >  #define HTTPD_FCGI_NAME_MAX    511
> > >  #define HTTPD_FCGI_VAL_MAX     511
> > > 
> > > I fixed the problem by sending the headers one after the other.
> > 
> > Isn't 511 bytes for a header a bit short? e.g. some cookies and JWT tokens
> > can be rather large.
> 
> Yes that's true. I upper the limits for header to 256/8192 and not to
> touch the IMSG limit for the time being.
> 
> > 
> > The imsg passing in config_setserver_headers() sends the tailq pointers
> > which breaks the fork/exec ASLR protection. I would suggest to use encode
> > the headers differently and not use the struct for passing. With that also
> > the fixed size string buffers can be changed to a length + str value
> > encoding.
> > 
> > You can probably shoehorn this into proc_composev, on the receive side
> > just use imsg_get_buf() and imsg_get_strbuf() to pull the data out.
> >  
> 
> Ohh! Thanks, fixed like this:
> 
> config_setserver_headers() now sends a small fixed struct header_imsg
> (id, flags, and the name/value lengths) followed by the raw name and
> value bytes via proc_composev()
> 
> As you suggested, the length encoding also let me drop the fixed
> name[]/value[] buffers in custom_header.
> 
> If you only interested in review this part, you can find it here
> https://rsadowski.gothub.org/?action=diff&commit=daa3667f40735c225a6ba94ebd6bb7bd90a115bc&headref=custom-http-header-support&path=httpd.git
> 
> If we can agree on that, I can and will implement ("fix") it for FastCGI
> as well.
> 
> Here is the full diff and another iteration with the following changes:
> 
>     httpd: apply "add ... always" headers to internal error responses
> 
>     Bring back get_always_custom_headers() to append "add ... always"
>     headers to those responses. Only "add" is handled here. "set" and
>     "remove" are ignored for now, so "set ... always" is dropped from the
>     grammar. Framing headers are already rejected at parse time, so a raw
>     append cannot desync the response, only produce a duplicate header.
> 
>     This is a stopgap. A later change should fold this into
>     server_custom_headers() so all header operations work the same way on
>     every response.
> 
> That should be "everything",

Looks good to me. Two minor comments.

- why is always only supported for add?
  Why can't I remove or set headers in error conditions?

- I would use simple #define for header_flags instead of the enum
> +enum header_flags {
> +	HEADER_REMOVE	= 0x01,
> +	HEADER_ADD	= 0x02,
> +	HEADER_SET	= 0x04,
> +	HEADER_ALWAYS	= 0x08
> +};

The enum is never used and using enum values as flags feels wrong to me.

-- 
:wq Claudio