Bug 58231 - RewriteCond can add "Host" to the Vary-Header
Summary: RewriteCond can add "Host" to the Vary-Header
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_rewrite (show other bugs)
Version: 2.4.27
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-11 11:59 UTC by Rainer Canavan
Modified: 2017-11-26 11:58 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rainer Canavan 2015-08-11 11:59:39 UTC
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
[,...]
Comment 1 Howard H. 2017-09-13 18:42:59 UTC
This still happens in 2.4.20, is there a fix or proposed fix?
Comment 2 Rainer Canavan 2017-09-14 09:34:59 UTC
Simplified Reproducer:

    RewriteCond %{HTTP_HOST} !whatever
    RewriteRule ^/a /b
Comment 3 Eric Covener 2017-09-14 11:39:34 UTC
(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?
Comment 4 Rainer Canavan 2017-09-15 08:52:26 UTC
> 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.
Comment 5 Rainer Canavan 2017-09-15 08:54:22 UTC
I've also tested this with 2.4.27 and it also adds "Host" to the Vary Header.
Comment 6 Luca Toscano 2017-09-16 11:25:38 UTC
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.
Comment 7 Luca Toscano 2017-09-18 08:00:28 UTC
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?
Comment 8 Luca Toscano 2017-09-18 13:11:36 UTC
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);
 }
Comment 9 Luca Toscano 2017-09-23 09:18:07 UTC
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.
Comment 10 Luca Toscano 2017-09-26 12:22:25 UTC
(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.
Comment 11 Luca Toscano 2017-09-28 12:11:15 UTC
(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)
Comment 12 Eric Covener 2017-09-28 12:37:03 UTC
(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?
Comment 13 Luca Toscano 2017-09-28 16:47:24 UTC
(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 :)
Comment 14 Luca Toscano 2017-09-29 08:01:02 UTC
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..
Comment 15 Yann Ylavic 2017-09-29 08:18:09 UTC
Maybe using err_headers_out (instead of headers_out) to set Vary in the first place?
Comment 16 Luca Toscano 2017-09-29 14:49:19 UTC
(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!
Comment 17 Luca Toscano 2017-09-30 09:31:48 UTC
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..)
Comment 18 Luca Toscano 2017-10-02 16:55:56 UTC
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 :)
Comment 19 Rainer Canavan 2017-10-02 17:26:49 UTC
> 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.
Comment 20 Luca Toscano 2017-10-02 17:37:09 UTC
(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.
Comment 21 Luca Toscano 2017-10-08 09:44:51 UTC
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.
Comment 22 Luca Toscano 2017-10-10 17:42:46 UTC
Fix proposal for trunk committed with r1811744
Comment 23 Luca Toscano 2017-10-18 15:48:20 UTC
Updated tests in r1812542 with the new code for the directory context, will propose a backport later on during the week.
Comment 24 Luca Toscano 2017-11-26 11:58:26 UTC
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 :)