Bug 2621: Provide request headers to RESPMOD when using cache_peer.

A short-term fix.

When FwdServer::_peer is set, HttpStateData constructor creates a new special
HttpRequest, overwriting the request pointer set in the parent
(ServerStateData) constructor to fwd->request.

To make matters worse, this special peer request has no headers at all (even
though flags and some cached/computed header values are copied). We initialize
it with the right URL, method, and protocol. We copy flags and a few other
random properties from the original request. We never copy the original
headers.

Furthermore, regardless of the peering, when we create the headers to send to
the next hop, those headers are temporary and not stored in any request
structure (see HttpStateData::buildRequestPrefix). The non-peering code
survives this because the request member points to fwd->request, which has the
headers. The peering code fails as illustrated by this bug.

I believe both cases are buggy because server-side adaptation and core code
should have access to the request headers we sent rather than the request
headers we received and adapted (or no headers at all). After all, it is the
sent headers that determine the next hop view of our Squid and adaptation
services should see a pair of _matching_ request and response headers.

I am pretty sure there are other bugs related to HttpStateData using a special
peer request structure instead of fwd->request. Please note that FwdState has
no idea that this substitution is going on.

This quick short-term fix uses the original request and its headers when
checking RESPMOD ACLs. This is what the patch in bug #2562 did for Squid v3.0.
For the reasons described above, this patch may be either insufficient or
wrong for the long-term fix.

diff --git a/src/Server.cc b/src/Server.cc
index 8256a1f77a2f9c6a45a8e154102d37a7d6a18d7d..af24ef83986bcdeacf78ae77f301448db7f35e24 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -833,7 +833,7 @@ ServerStateData::adaptOrFinalizeReply()
     // The callback can be called with a NULL service if adaptation is off.
     adaptationAccessCheckPending = Adaptation::AccessCheck::Start(
                                        Adaptation::methodRespmod, Adaptation::pointPreCache,
-                                       request, virginReply(), adaptationAclCheckDoneWrapper, this);
+                                       originalRequest(), virginReply(), adaptationAclCheckDoneWrapper, this);
     debugs(11,5, HERE << "adaptationAccessCheckPending=" << adaptationAccessCheckPending);
     if (adaptationAccessCheckPending)
         return;

diff --git a/src/http.cc b/src/http.cc
index 87718c7f616f7eda89b2b31c02f12fa6afea9053..a96bbb259494f2e89adf77192f77279ccee8f5ad 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -769,7 +769,7 @@ HttpStateData::handle1xx(HttpReply *reply)
 #if USE_HTTP_VIOLATIONS
     // check whether the 1xx response forwarding is allowed by squid.conf
     if (Config.accessList.reply) {
-        ACLFilledChecklist ch(Config.accessList.reply, request, NULL);
+        ACLFilledChecklist ch(Config.accessList.reply, originalRequest(), NULL);
         ch.reply = HTTPMSGLOCK(reply);
         if (!ch.fastCheck()) { // TODO: support slow lookups?
             debugs(11, 3, HERE << "ignoring denied 1xx");
@@ -2208,7 +2208,7 @@ HttpStateData::finishingBrokenPost()
         return false;
     }
 
-    ACLFilledChecklist ch(Config.accessList.brokenPosts, request, NULL);
+    ACLFilledChecklist ch(Config.accessList.brokenPosts, originalRequest(), NULL);
     if (!ch.fastCheck()) {
         debugs(11, 5, HERE << "didn't match brokenPosts");
         return false;