A RewriteCond that evaluates %{HTTP_HOST} automatically adds "Host" to the Vary-Header. This is unnecessary and not permitted according to https://tools.ietf.org/html/rfc7231#section-7.1.4 > The "Vary" header field in a response describes what parts of a request message, aside from the method, Host header field, and request target, might influence the origin server's process for selecting and representing this response. The RewriteRule that triggered the Vary: Host for me is below, however, I'm certain simpler examples can be constructed: RewriteCond expr "%{HTTP_REFERER} =~ m#^https?://([^/:]+)# && %{HTTP_HOST}.':' -strmatch '$1:*'" RewriteRule ^ - [env=FOO:bar] > GET /foo/lenna/ HTTP/1.1 > User-Agent: curl/7.38.0 > Host: pahuanuiapitaaitera.office.sevenval.de:14020 > Accept: */* > Referer: http://$1/ > < HTTP/1.1 200 OK < Date: Tue, 11 Aug 2015 11:58:01 GMT < Server: Apache < Vary: User-Agent,Referer,Host,Accept-Encoding [,...]
This still happens in 2.4.20, is there a fix or proposed fix?
Simplified Reproducer: RewriteCond %{HTTP_HOST} !whatever RewriteRule ^/a /b
(In reply to Rainer Canavan from comment #0) > A RewriteCond that evaluates %{HTTP_HOST} automatically adds "Host" to the > Vary-Header. This is unnecessary and not permitted according to > https://tools.ietf.org/html/rfc7231#section-7.1.4 Can you quote the "not permitted" part? Is there any practical harm in your scenario?
> Can you quote the "not permitted" part? I would interpret the very first sentence as implying that the host header is at least not expected to be listed in Vary: > The "Vary" header field in a response describes what parts of a request message, aside from the method, Host header field, and request target, might influence [...] > Is there any practical harm in your scenario? It's been a while, so I can't recall the details, but we've encountered at least one proxy or client that refused to cache responses if "Host" was listen in the Vary Header.
I've also tested this with 2.4.27 and it also adds "Host" to the Vary Header.
From mod_rewrite.c in httpd-trunk I can see the following: /* * lookup a HTTP header and set VARY note */ static const char *lookup_header(const char *name, rewrite_ctx *ctx) { const char *val = apr_table_get(ctx->r->headers_in, name); if (val) { ctx->vary_this = ctx->vary_this ? apr_pstrcat(ctx->r->pool, ctx->vary_this, ", ", name, NULL) : apr_pstrdup(ctx->r->pool, name); } return val; } So it seems to be done on purpose but I didn't find anything useful in the svn commit log. Will spend a bit of time during the next days and report if I find anything useful.
I found the following example as proof of issues originated from the Vary: Host: http://www.nivas.hr/blog/2017/02/13/apache-sending-vary-host-making-things-uncacheable-varnish/ I still haven't been able to get a response with Vary:Host using mod_rewrite (although I can see from the code that it explicitly set if when it finds it in a RewriteCond, so probably I am doing something wrong in my config) but I found another way to get it: <If '%{HTTP_HOST} -eq "bla"'> [..] </If> It makes sense since I can see a add_vary function in util_expr_eval.c. As far as I understand the RFC explicitly prohibits the Vary: Host header, so it should be prevented in out codebase. Any opinion?
The following seems to remove the Vary: Host header combination from mod_rewrite and <If> expression evaluation: Index: modules/mappers/mod_rewrite.c =================================================================== --- modules/mappers/mod_rewrite.c (revision 1808672) +++ modules/mappers/mod_rewrite.c (working copy) @@ -2035,7 +2035,10 @@ case 'S': if (!strcmp(var, "HTTP_HOST")) { - result = lookup_header("Host", ctx); + /* Skip the 'Vary: Host' header combination + * as indicated in rfc7231 section-7.1.4 + */ + result = apr_table_get(ctx->r->headers_in, "Host"); } break; Index: server/util_expr_eval.c =================================================================== --- server/util_expr_eval.c (revision 1808672) +++ server/util_expr_eval.c (working copy) @@ -1606,7 +1606,12 @@ return ""; name = req_header_header_names[index]; - add_vary(ctx, name); + /* Skip the 'Vary: Host' header combination + * as indicated in rfc7231 section-7.1.4 + */ + if (strcmp(name, "Host")){ + add_vary(ctx, name); + } return apr_table_get(ctx->r->headers_in, name); }
Fixed in trunk with: http://svn.apache.org/r1808746 http://svn.apache.org/r1809028 I am going to write some tests and then propose the fix for 2.4.x.
(In reply to Rainer Canavan from comment #2) > Simplified Reproducer: > > RewriteCond %{HTTP_HOST} !whatever > RewriteRule ^/a /b While writing tests I discovered that the above configuration does not trigger the issue when embedded in a Directory block (with proper stripping of starting "/" that don't match) because afaics an internal rewrite happens that clears out the outstanding Vary values.
(In reply to Luca Toscano from comment #10) > While writing tests I discovered that the above configuration does not > trigger the issue when embedded in a Directory block (with proper stripping > of starting "/" that don't match) because afaics an internal rewrite happens > that clears out the outstanding Vary values. This seems to be a long standing issue already reported by others, and essentially due to the ap_redirect_internal function not carrying any output headers already set in the "pre-ap_redirect_internal" request. The following patch seems to work fine with RewriteConds in Directory context: Index: modules/http/http_request.c =================================================================== --- modules/http/http_request.c (revision 1809976) +++ modules/http/http_request.c (working copy) @@ -523,6 +523,7 @@ request_rec *r) { int access_status; request_rec *new; + const char *vary_headers; if (ap_is_recursion_limit_exceeded(r)) { ap_die(HTTP_INTERNAL_SERVER_ERROR, r); @@ -581,6 +582,12 @@ new->headers_in = r->headers_in; new->trailers_in = r->trailers_in; new->headers_out = apr_table_make(r->pool, 12); + + vary_headers = apr_table_get(r->headers_out, "Vary"); + if(vary_headers) { + apr_table_setn(new->headers_out, "Vary", vary_headers); + } + if (ap_is_HTTP_REDIRECT(new->status)) { const char *location = apr_table_get(r->headers_out, "Location"); if (location)
(In reply to Luca Toscano from comment #11) > (In reply to Luca Toscano from comment #10) > > While writing tests I discovered that the above configuration does not > > trigger the issue when embedded in a Directory block (with proper stripping > > of starting "/" that don't match) because afaics an internal rewrite happens > > that clears out the outstanding Vary values. > > This seems to be a long standing issue already reported by others, and > essentially due to the ap_redirect_internal function not carrying any output > headers already set in the "pre-ap_redirect_internal" request. The following > patch seems to work fine with RewriteConds in Directory context: > > Index: modules/http/http_request.c > =================================================================== > --- modules/http/http_request.c (revision 1809976) > +++ modules/http/http_request.c (working copy) > @@ -523,6 +523,7 @@ > request_rec *r) { > int access_status; > request_rec *new; > + const char *vary_headers; > > if (ap_is_recursion_limit_exceeded(r)) { > ap_die(HTTP_INTERNAL_SERVER_ERROR, r); > @@ -581,6 +582,12 @@ > new->headers_in = r->headers_in; > new->trailers_in = r->trailers_in; > new->headers_out = apr_table_make(r->pool, 12); > + > + vary_headers = apr_table_get(r->headers_out, "Vary"); > + if(vary_headers) { > + apr_table_setn(new->headers_out, "Vary", vary_headers); > + } > + > if (ap_is_HTTP_REDIRECT(new->status)) { > const char *location = apr_table_get(r->headers_out, "Location"); > if (location) Doesn't look so safe as a behavior change. It's probably sensible in this scenario but I could see this breaking people. Could rewrite add the varies after the internal redirect?
(In reply to Eric Covener from comment #12) > Doesn't look so safe as a behavior change. It's probably sensible in this > scenario but I could see this breaking people. Could rewrite add the varies > after the internal redirect? Thanks for the feedback Eric, I have the same feeling. Going to test other way to make this change :)
I tried some quick tests but it seems that when mod_rewrite hands over the request to ap_internal_redirect then it is not going to be able to add Vary headers anymore. It would be great to have a way to instruct the ap_internal_redirect that some response headers needs to be "forwarded" to the new response..
Maybe using err_headers_out (instead of headers_out) to set Vary in the first place?
(In reply to Yann Ylavic from comment #15) > Maybe using err_headers_out (instead of headers_out) to set Vary in the > first place? Yann you are awesome as usual, the following seems working perfectly: Index: modules/mappers/mod_rewrite.c =================================================================== --- modules/mappers/mod_rewrite.c (revision 1810117) +++ modules/mappers/mod_rewrite.c (working copy) @@ -5276,6 +5276,8 @@ */ static int handler_redirect(request_rec *r) { + const char *vary_headers; + if (strcmp(r->handler, REWRITE_REDIRECT_HANDLER_NAME)) { return DECLINED; } @@ -5285,6 +5287,12 @@ return DECLINED; } + vary_headers = apr_table_get(r->headers_out, "Vary"); + + if(vary_headers) { + apr_table_setn(r->err_headers_out, "Vary", vary_headers); + } + /* now do the internal redirect */ ap_internal_redirect(apr_pstrcat(r->pool, r->filename+9, r->args ? "?" : NULL, r->args, NULL), r); Will do more tests (and probably improve the code) but it seems really promising! Thanks!
This solution has a drawback, namely that even the error responses will get the Vary header. I tested a simple RewriteRule configured to lead to a 404, and it gets the Vary header (that is expected using err_output_headers from what I can read..)
Another solution that I played with (just a hack, nothing complete) is the following: Index: modules/http/http_request.c =================================================================== --- modules/http/http_request.c (revision 1810323) +++ modules/http/http_request.c (working copy) @@ -523,6 +523,8 @@ request_rec *r) { int access_status; request_rec *new; + const char *header_to_copy = NULL; + const char *header_values_to_copy = NULL; if (ap_is_recursion_limit_exceeded(r)) { ap_die(HTTP_INTERNAL_SERVER_ERROR, r); @@ -586,6 +588,13 @@ if (location) apr_table_setn(new->headers_out, "Location", location); } + + if ((header_to_copy = apr_table_get(r->subprocess_env, "keep-response-header"))) { + if((header_values_to_copy = apr_table_get(r->headers_out, header_to_copy))) { + apr_table_setn(new->headers_out, header_to_copy, header_values_to_copy); + } + } + new->err_headers_out = r->err_headers_out; new->trailers_out = apr_table_make(r->pool, 5); new->subprocess_env = rename_original_env(r->pool, r->subprocess_env); Index: modules/mappers/mod_rewrite.c =================================================================== --- modules/mappers/mod_rewrite.c (revision 1810323) +++ modules/mappers/mod_rewrite.c (working copy) @@ -5285,6 +5285,8 @@ return DECLINED; } + apr_table_setn(r->subprocess_env, "keep-response-header", "Vary"); + /* now do the internal redirect */ ap_internal_redirect(apr_pstrcat(r->pool, r->filename+9, r->args ? "?" : NULL, r->args, NULL), r); This allows to selectively merge some headers from r->headers_out to the new request, without: 1) impacting existing code using the ap_internal_redirect since only mod_rewrite would use it. 2) force error responses after ap_internal_redirect to carry those headers (like the vary ones). Not sure if it makes any sense, but I am running out of ideas :)
> 2) force error responses after ap_internal_redirect to carry those headers (like the vary ones). I'm not sure if I understand that correctly, but I'd expect error responses to come with a Vary-Header, if any request header was evaluated to arrive at that error. After all, 404 and friends are cacheable by default.
(In reply to Rainer Canavan from comment #19) > > 2) force error responses after ap_internal_redirect to carry those headers (like the vary ones). > > I'm not sure if I understand that correctly, but I'd expect error responses > to come with a Vary-Header, if any request header was evaluated to arrive > at that error. After all, 404 and friends are cacheable by default. Sure but probably not a 50X, 404 might be borderline but afaict the current behavior for mod_rewrite (adding automatically Vary headers when needed) is not to add Vary headers in any error response, so I'd be inclined to keep this behavior.
A simpler variant of the patch: Index: modules/http/http_request.c =================================================================== --- modules/http/http_request.c (revision 1811460) +++ modules/http/http_request.c (working copy) @@ -523,6 +523,7 @@ request_rec *r) { int access_status; request_rec *new; + const char *vary_header = NULL; if (ap_is_recursion_limit_exceeded(r)) { ap_die(HTTP_INTERNAL_SERVER_ERROR, r); @@ -586,6 +587,13 @@ if (location) apr_table_setn(new->headers_out, "Location", location); } + + if (apr_table_get(r->notes, "keep-vary-header")) { + if((vary_header = apr_table_get(r->headers_out, "Vary"))) { + apr_table_setn(new->headers_out, "Vary", vary_header); + } + } + new->err_headers_out = r->err_headers_out; new->trailers_out = apr_table_make(r->pool, 5); new->subprocess_env = rename_original_env(r->pool, r->subprocess_env); Index: modules/mappers/mod_rewrite.c =================================================================== --- modules/mappers/mod_rewrite.c (revision 1811460) +++ modules/mappers/mod_rewrite.c (working copy) @@ -5226,6 +5226,8 @@ } } + apr_table_setn(r->notes, "keep-vary-header", ""); + /* now initiate the internal redirect */ rewritelog((r, 1, dconf->directory, "internal redirect with %s " "[INTERNAL REDIRECT]", r->filename)); This one uses 'notes' rather than 'subprocess_env' and it is simpler. All my tests passes, will ask some feedback to other devs about the best way to proceed.
Fix proposal for trunk committed with r1811744
Updated tests in r1812542 with the new code for the directory context, will propose a backport later on during the week.
Backport committed in r1815100. I don't see more actionable for this task, closing it, thanks everybody for the help! And please re-open if I am missing something :)