From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 30 Mar 2011 12:02:11 +0000 (-0600)
Subject: Bug 2621: Provide request headers to RESPMOD when using cache_peer.
X-Git-Tag: SQUID_3_1_12~19
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5d81c693c64c517eb8d5e62326c605fdad8c3a6b;p=thirdparty%2Fsquid.git

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 990cf819d4..cdc2a2da53 100644
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -815,7 +815,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 646dd728f3..7689057a5b 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -2076,7 +2076,7 @@ HttpStateData::doneSendingRequestBody()
 
 #if HTTP_VIOLATIONS
     if (Config.accessList.brokenPosts) {
-        ACLFilledChecklist ch(Config.accessList.brokenPosts, request, NULL);
+        ACLFilledChecklist ch(Config.accessList.brokenPosts, originalRequest(), NULL);
         if (!ch.fastCheck()) {
             debugs(11, 5, "doneSendingRequestBody: didn't match brokenPosts");
             CommIoCbParams io(NULL);