By default do not deliver failed 200 ESI:includes.

582ded6a · Poul-Henning Kamp · 2c33188b · 582ded6a · 582ded6a · 582ded6a
Commit 582ded6a authored Jan 31, 2023 by Poul-Henning Kamp
Showing with 95 additions and 14 deletions

cache_esi_deliver.c bin/varnishd/cache/cache_esi_deliver.c +13 -5

r03241.vtc bin/varnishtest/tests/r03241.vtc +6 -9

r03865.vtc bin/varnishtest/tests/r03865.vtc +57 -0

changes.rst doc/changes.rst +19 -0

No files found.
--- a/bin/varnishd/cache/cache_esi_deliver.c
+++ b/bin/varnishd/cache/cache_esi_deliver.c
@@ -65,7 +65,7 @@ struct ecx {
 	ssize_t		l;
 	int		isgzip;
 	int		woken;
-	int		abrt;
+	int		incl_cont;

 	struct req	*preq;
 	struct ecx	*pecx;
@@ -382,11 +382,11 @@ ved_vdp_esi_bytes(struct vdp_ctx *vdx, enum vdp_action act, void **priv,
 				Debug("SKIP1(%d)\n", (int)ecx->l);
 				ecx->state = 4;
 				break;
-			case VEC_IA:
-				ecx->abrt =
+			case VEC_IC:
+				ecx->incl_cont =
 				    FEATURE(FEATURE_ESI_INCLUDE_ONERROR);
 				/* FALLTHROUGH */
-			case VEC_IC:
+			case VEC_IA:
 				ecx->p++;
 				q = (void*)strchr((const char*)ecx->p, '\0');
 				AN(q);
@@ -869,6 +869,14 @@ ved_deliver(struct req *req, struct boc *boc, int wantbody)
 	if (wantbody == 0)
 		return;

+	if (!ecx->incl_cont &&
+	    req->resp->status != 200 &&
+	    req->resp->status != 204) {
+		req->top->topreq->vdc->retval = -1;
+		req->top->topreq->doclose = req->doclose;
+		return;
+	}
+
 	if (boc == NULL && ObjGetLen(req->wrk, req->objcore) == 0)
 		return;

@@ -918,7 +926,7 @@ ved_deliver(struct req *req, struct boc *boc, int wantbody)

 	req->acct.resp_bodybytes += VDP_Close(req->vdc);

-	if (i && ecx->abrt) {
+	if (i && !ecx->incl_cont) {
 		req->top->topreq->vdc->retval = -1;
 		req->top->topreq->doclose = req->doclose;
 	}

--- a/bin/varnishtest/tests/r03241.vtc
+++ b/bin/varnishtest/tests/r03241.vtc
@@ -4,12 +4,8 @@ varnishtest "ESI include out of workspace"
 server s1 {
 	rxreq
 	expect req.http.esi0 == "foo"
-	txresp -body {
-		<html>
-		Before include
-		<esi:include src="/body" sr="foo"/>
-		After include
-		</html>
+	txresp -body {<html>Before include<esi:include
+		src="/body" sr="foo"/>After include</html>
 	}
 	rxreq
 	expect req.url == "/body1"
@@ -47,10 +43,11 @@ logexpect l1 -v v1 -g raw {

 client c1 {
 	txreq -hdr "Host: foo"
-	rxresp
-	# XXX this is actually wrong (missed include)
-	expect resp.bodylen == 57
+	rxresphdrs
 	expect resp.status == 200
+	rxchunk
+	expect_close
+	expect resp.body == {<html>Before include}
 }  -run

 logexpect l1 -wait
--- a/bin/varnishtest/tests/r03865.vtc
+++ b/bin/varnishtest/tests/r03865.vtc
+varnishtest "ESI onerror"
+
+server s1 {
+	rxreq
+	expect req.url == "/abort"
+	txresp -hdr {surrogate-control: content="ESI/1.0"} \
+	    -body {before <esi:include src="/fail" onerror="abort"/> after}
+
+} -start
+
+varnish v1 -cliok "param.set feature +esi_disable_xml_check"
+varnish v1 -cliok "param.set feature +esi_include_onerror"
+varnish v1 -vcl+backend {
+	sub vcl_backend_fetch {
+		if (bereq.url == "/fail") {
+			return (error(604));
+		}
+	}
+	sub vcl_backend_response {
+		set beresp.do_esi = beresp.http.surrogate-control ~ "ESI/1.0";
+		unset beresp.http.surrogate-control;
+	}
+	sub vcl_backend_error {
+		if (beresp.status == 604) {
+			set beresp.body = "FOOBAR";
+			return(deliver);
+		}
+	}
+} -start
+
+client c1 {
+	txreq -url "/abort"
+	non_fatal
+	rxresphdrs
+	expect resp.status == 200
+	rxchunk
+	expect_close
+	expect resp.body == "before "
+} -run
+
+varnish v1 -vsl_catchup
+
+server s1 -wait
+
+server s1 {
+	rxreq
+	expect req.url == "/continue"
+	txresp -hdr {surrogate-control: content="ESI/1.0"} \
+	    -body {before <esi:include src="/fail" onerror="continue"/> after}
+} -start
+
+client c1 {
+	fatal
+	txreq -url "/continue"
+	rxresp
+	expect resp.body == "before FOOBAR after"
+} -run
--- a/doc/changes.rst
+++ b/doc/changes.rst
@@ -35,6 +35,25 @@ release process.
 Varnish Cache NEXT (2023-03-15)
 ===============================

+* Do not ESI:include failed objects unless instructed to.
+
+  Previously, any ESI:include object would be included, no matter
+  what the status of it were, 200, 503, didn't matter.
+
+  From now on, by default, only objects with 200 and 204 status
+  will be included and any other status code will fail the parent
+  ESI request.
+
+  If objects with other status should be delivered, they should
+  have their status changed to 200 in VCL, for instance in
+  ``sub vcl_backend_error{}``, ``vcl_synth{}`` or ``vcl_deliver{}``.
+
+  If ``param.set feature +esi_include_onerror`` is used, and the
+  ``<esi:include …>`` tag has a ``onerror="continue"`` attribute,
+  any and all ESI:include objects will be delivered, no matter what
+  their status might be, and not even a partial delivery of them
+  will fail the parent ESI request.  To be used with great caution.
+
 * VXIDs are 64 bit now and the binary format of SHM and raw saved
  VSL files has changed as a consequence.