[master] 2b3c76a84 resp.do_esi is undefined after setting resp.filters

[Nils Goroll]

commit 2b3c76a843207324bb2f3a3ece5e3d36881dfc52
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Sun Apr 5 14:48:28 2020 +0200

    resp.do_esi is undefined after setting resp.filters
    
    After fixing resp.filters, resp.do_esi loses its meaning.
    
    Note:
    
    Setting resp.filters also fixes whether or not the gunzip and range VDPs
    are being pushed.
    
    These do not depend on switches, but on request headers. So we need to
    consider the case that a VCL author
    
            - fixes resp.filters
            - and then adds or removes relevant headers
    
    * req.http.Accept-Encoding for the gunzip vdp
    
      if removed: gunzip stays in the VDP list, which is always ok
    
      if added: that is probably wrong anyway and the VCL author can
      be held responsible for their actions as per the warning in the
      resp.filters documentation.
    
    * req.http.Range for the range vdp
    
      if removed: the range vdp vetoes itself in the _init callback
    
      if added: no range handling will be present, which is always ok
      (and see above for VCL author responsibilities)
    
    Fixes #3002

diff --git a/bin/varnishd/cache/cache_vrt_var.c b/bin/varnishd/cache/cache_vrt_var.c
index a911a3622..8782df08d 100644
--- a/bin/varnishd/cache/cache_vrt_var.c
+++ b/bin/varnishd/cache/cache_vrt_var.c
@@ -869,6 +869,15 @@ VRT_r_resp_is_streaming(VRT_CTX)
 
 /*--------------------------------------------------------------------*/
 
+static inline int
+resp_filter_fixed(VRT_CTX, const char *s)
+{
+	if (ctx->req->filter_list == NULL)
+		return (0);
+	VRT_fail(ctx, "resp.filters are already fixed, %s is undefined", s);
+	return (1);
+}
+
 VCL_VOID
 VRT_l_resp_do_esi(VRT_CTX, VCL_BOOL process_esi)
 {
@@ -876,6 +885,8 @@ VRT_l_resp_do_esi(VRT_CTX, VCL_BOOL process_esi)
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
 	assert(ctx->syntax >= 41);
+	if (resp_filter_fixed(ctx, "resp.do_esi"))
+		return;
 	ctx->req->disable_esi = !process_esi;
 }
 
@@ -886,6 +897,8 @@ VRT_r_resp_do_esi(VRT_CTX)
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
 	assert(ctx->syntax >= 41);
+	if (resp_filter_fixed(ctx, "resp.do_esi"))
+		return (0);
 	return (!ctx->req->disable_esi);
 }
 
diff --git a/bin/varnishtest/tests/e00015.vtc b/bin/varnishtest/tests/e00015.vtc
index 4df48cec0..951785115 100644
--- a/bin/varnishtest/tests/e00015.vtc
+++ b/bin/varnishtest/tests/e00015.vtc
@@ -71,10 +71,15 @@ server s1 {
 varnish v1 -syntax 4.1 -vcl+backend {
 	sub vcl_deliver {
 		set resp.http.was = resp.do_esi;
+		set resp.http.filter0 = resp.filters;
 		if (req.url == "/top2") {
 			set resp.do_esi = false;
 		}
 		set resp.http.filters = resp.filters;
+		if (req.http.fiddle) {
+			set resp.filters = resp.filters;
+			set resp.do_esi = false;
+		}
 	}
 	sub vcl_backend_response {
 		set beresp.do_esi = true;
@@ -89,6 +94,7 @@ client c1 {
 	expect resp.bodylen == 73
 	expect resp.status == 200
 	expect resp.http.was == true
+	expect resp.http.filter0 == "esi gunzip"
 	expect resp.http.filters == "gunzip"
 
 	txreq -url "/esi"
@@ -96,6 +102,7 @@ client c1 {
 	expect resp.bodylen == 76
 	expect resp.status == 200
 	expect resp.http.was == true
+	expect resp.http.filter0 == "esi"
 	expect resp.http.filters == "esi"
 
 	# see Note on Range above
@@ -120,6 +127,10 @@ client c1 {
 	expect resp.status == 200
 	expect resp.http.was == true
 	expect resp.http.filters == "esi gunzip range"
+
+	txreq -url /top2 -hdr "fiddle: do_esi"
+	rxresp
+	expect resp.status == 503
 } -run
 
 varnish v1 -expect esi_errors == 0
diff --git a/doc/sphinx/reference/vcl_var.rst b/doc/sphinx/reference/vcl_var.rst
index a26c6d7ff..93d0414b8 100644
--- a/doc/sphinx/reference/vcl_var.rst
+++ b/doc/sphinx/reference/vcl_var.rst
@@ -1218,6 +1218,8 @@ resp.http.*
 
 	The HTTP headers that will be returned.
 
+.. XXX does vcl_synth make any sense?
+
 resp.do_esi	``VCL >= 4.1``
 
 	Type: BOOL
@@ -1226,12 +1228,14 @@ resp.do_esi	``VCL >= 4.1``
 
 	Writable from: vcl_deliver, vcl_synth
 
-	Default: Set if ESI parsing has happened.
+	Default: obj.can_esi
 
 	This can be used to selectively disable ESI processing,
 	even though ESI parsing happened during fetch.
 	This is useful when Varnish caches peer with each other.
 
+	It is a VCL error to use resp.do_esi after setting resp.filters.
+
 
 resp.is_streaming
 
@@ -1252,6 +1256,15 @@ resp.filters
 
 	List of VDP filters the resp.body will be pushed through.
 
+	Before resp.filters is set, the value read will be the default
+	filter list as determined by varnish based on resp.do_esi and
+	request headers.
+
+	After resp.filters is set, changing any of the conditions
+	which otherwise determine the filter selection will have no
+	effiect. Using resp.do_esi is an error once resp.filters is
+	set.
+
 Special variables
 ~~~~~~~~~~~~~~~~~