[PATCH 05 of 20] Combining unknown headers during variables lookup (ticket #1316)

Maxim Dounin mdounin at mdounin.ru
Thu May 12 23:15:17 UTC 2022


Hello!

On Wed, May 11, 2022 at 08:09:10PM +0400, Sergey Kandaurov wrote:

> On Thu, Apr 21, 2022 at 01:18:45AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1650492321 -10800
> > #      Thu Apr 21 01:05:21 2022 +0300
> > # Node ID 2ea48b5e4643a818cd81179f040f0b36be9050d6
> > # Parent  3618f35ac3c8833cfd4a4c80295ab94eb924a9a7
> > Combining unknown headers during variables lookup (ticket #1316).
> > 
> > Previously, $http_*, $sent_http_*, $upstream_http_*, and $upstream_trailer_*
> > variables returned only the first header (with a few specially handled
> > exceptions: $http_cookie, $http_x_forwarded_for, $sent_http_cache_control,
> > $sent_http_link).
> 
> forgot to mention "sent_trailer_"

Yes, thanks, added to the list.

> jftr, this is an API change, used at least in mod_zip for for "upstream_http_"

In the particular case this rather looks like a misuse of the 
ngx_http_variable_unknown_header() function in the mod_zip module, 
as it clearly does not use it to lookup the $upstream_http_* 
variable, but instead tries to emulate such a lookup.

Either way, this series introduces quite a few API changes, so it 
doesn't really matter.

> > 
> > With this change, all headers are returned, combined together.  For
> > example, $http_foo variable will be "a, b" if there are "Foo: a" and
> > "Foo: b" headers in the request.
> > 
> > Note that $upstream_http_set_cookie will also return all "Set-Cookie"
> > headers (ticket #1843), though this might not be what one want, since
> > the "Set-Cookie" header does not follow the list syntax (see RFC 7230,
> > section 3.2.2).
> > 
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -5703,7 +5703,7 @@ ngx_http_upstream_header_variable(ngx_ht
> >          return NGX_OK;
> >      }
> >  
> > -    return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> > +    return ngx_http_variable_unknown_header(r, v, (ngx_str_t *) data,
> >                                           &r->upstream->headers_in.headers.part,
> >                                           sizeof("upstream_http_") - 1);
> >  }
> > @@ -5718,7 +5718,7 @@ ngx_http_upstream_trailer_variable(ngx_h
> >          return NGX_OK;
> >      }
> >  
> > -    return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> > +    return ngx_http_variable_unknown_header(r, v, (ngx_str_t *) data,
> >                                          &r->upstream->headers_in.trailers.part,
> >                                          sizeof("upstream_trailer_") - 1);
> >  }
> > diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
> > --- a/src/http/ngx_http_variables.c
> > +++ b/src/http/ngx_http_variables.c
> > @@ -919,7 +919,7 @@ static ngx_int_t
> >  ngx_http_variable_unknown_header_in(ngx_http_request_t *r,
> >      ngx_http_variable_value_t *v, uintptr_t data)
> >  {
> > -    return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> > +    return ngx_http_variable_unknown_header(r, v, (ngx_str_t *) data,
> >                                              &r->headers_in.headers.part,
> >                                              sizeof("http_") - 1);
> >  }
> > @@ -929,7 +929,7 @@ static ngx_int_t
> >  ngx_http_variable_unknown_header_out(ngx_http_request_t *r,
> >      ngx_http_variable_value_t *v, uintptr_t data)
> >  {
> > -    return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> > +    return ngx_http_variable_unknown_header(r, v, (ngx_str_t *) data,
> >                                              &r->headers_out.headers.part,
> >                                              sizeof("sent_http_") - 1);
> >  }
> > @@ -939,19 +939,26 @@ static ngx_int_t
> >  ngx_http_variable_unknown_trailer_out(ngx_http_request_t *r,
> >      ngx_http_variable_value_t *v, uintptr_t data)
> >  {
> > -    return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> > +    return ngx_http_variable_unknown_header(r, v, (ngx_str_t *) data,
> >                                              &r->headers_out.trailers.part,
> >                                              sizeof("sent_trailer_") - 1);
> >  }
> >  
> >  
> >  ngx_int_t
> > -ngx_http_variable_unknown_header(ngx_http_variable_value_t *v, ngx_str_t *var,
> > +ngx_http_variable_unknown_header(ngx_http_request_t *r,
> > +    ngx_http_variable_value_t *v, ngx_str_t *var,
> >      ngx_list_part_t *part, size_t prefix)
> >  {
> > -    u_char            ch;
> > +    u_char           *p, ch;
> > +    size_t            len;
> >      ngx_uint_t        i, n;
> > -    ngx_table_elt_t  *header;
> > +    ngx_table_elt_t  *header, *h, **ph;
> > +
> > +    ph = &h;
> > +#if (NGX_SUPPRESS_WARN)
> > +    len = 0;
> > +#endif
> >  
> >      header = part->elts;
> >  
> > @@ -971,7 +978,11 @@ ngx_http_variable_unknown_header(ngx_htt
> >              continue;
> >          }
> >  
> > -        for (n = 0; n + prefix < var->len && n < header[i].key.len; n++) {
> > +        if (header[i].key.len != var->len - prefix) {
> > +            continue;
> > +        }
> > +
> > +        for (n = 0; n < var->len - prefix; n++) {
> >              ch = header[i].key.data[n];
> >  
> >              if (ch >= 'A' && ch <= 'Z') {
> > @@ -986,18 +997,59 @@ ngx_http_variable_unknown_header(ngx_htt
> >              }
> >          }
> >  
> > -        if (n + prefix == var->len && n == header[i].key.len) {
> > -            v->len = header[i].value.len;
> > -            v->valid = 1;
> > -            v->no_cacheable = 0;
> > -            v->not_found = 0;
> > -            v->data = header[i].value.data;
> > -
> > -            return NGX_OK;
> > +        if (n != var->len - prefix) {
> > +            continue;
> >          }
> > +
> > +        len += header[i].value.len + 2;
> > +
> > +        *ph = &header[i];
> > +        ph = &header[i].next;
> >      }
> >  
> > -    v->not_found = 1;
> > +    *ph = NULL;
> > +
> > +    if (h == NULL) {
> > +        v->not_found = 1;
> > +        return NGX_OK;
> > +    }
> > +
> > +    len -= 2;
> > +
> > +    if (h->next == NULL) {
> > +
> > +        v->len = h->value.len;
> 
> could be just len for consistency with common case

I think it's better to keep it straight: if there are no 
additional headers, we just return the only header.  And this 
matches the code in ngx_http_variable_headers_internal().

> 
> > +        v->valid = 1;
> > +        v->no_cacheable = 0;
> > +        v->not_found = 0;
> > +        v->data = h->value.data;
> > +
> > +        return NGX_OK;
> > +    }
> > +
> > +    p = ngx_pnalloc(r->pool, len);
> > +    if (p == NULL) {
> > +        return NGX_ERROR;
> > +    }
> > +
> > +    v->len = len;
> > +    v->valid = 1;
> > +    v->no_cacheable = 0;
> > +    v->not_found = 0;
> > +    v->data = p;
> > +
> > +    for ( ;; ) {
> > +
> > +        p = ngx_copy(p, h->value.data, h->value.len);
> > +
> > +        if (h->next == NULL) {
> > +            break;
> > +        }
> > +
> > +        *p++ = ','; *p++ = ' ';
> 
> what about Set-Cookie separator ";" support?
> looks like can be easly integrated (on top off):

There is no valid separator for the Set-Cookie header, multiple 
Set-Cookie headers cannot be represented as a single string.  The 
";" character works for the Cookie header, but not for Set-Cookie, 
which uses ";" to separate attribute-value pairs (as already 
mentioned in the commit log, see RFC 7230, section 3.2.2).
 
[...]

-- 
Maxim Dounin
http://mdounin.ru/



More information about the nginx-devel mailing list