Bug 419: Hop by Hop headers MUST NOT be forwarded (attempt 2)

This attempt builds on Henriks re-work of the client-request to
server-request cloning done since the last attempt was made at closing
this bug.

Adds all RFC 2616 listed Hop-by-hop headers to the clone selection test
as 'ignore' cases unless otherwise handled already.

The test for whether they exist in Connection: is moved to the default
case as an inline. Which reduces the code a fair bit and prevents the
side case where a specially handled header gets ignored because the
client explicitly added it to Connection: when it did not have to.

This method sets up a background default of not passing the hop-by-hop
headers while allowing any code which explicitly sets or copies the
headers across to operate as before without interference.

diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc
index aa7a7cf76cebd5df77ce05a2b208aaa41989749b..0384d8b0b27dd6a641fb8305a43733d93f55c5b5 100644 (file)
--- a/src/HttpHeaderTools.cc
+++ b/src/HttpHeaderTools.cc
@@ -182,7 +182,7 @@ httpHeaderHasConnDir(const HttpHeader * hdr, const char *directive)
     return res;
 }
 
-/* returns true iff "m" is a member of the list */
+/** returns true iff "m" is a member of the list */
 int
 strListIsMember(const String * list, const char *m, char del)
 {

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index f921a854e843e92bb64a39c8535295bbce3d41c3..f74dd0c453382f0fe305f4fd40d0c3ceb1b25832 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -326,6 +326,7 @@ HttpRequest::prefixLen()
            header.len + 2;
 }
 
+#if DEAD_CODE // 2009-01-20: inlined this with its ONLY caller (copyOneHeader...)
 /**
  * Returns true if HTTP allows us to pass this header on.  Does not
  * check anonymizer (aka header_access) configuration.
@@ -341,6 +342,7 @@ httpRequestHdrAllowed(const HttpHeaderEntry * e, String * strConn)
 
     return 1;
 }
+#endif
 
 /* sync this routine when you update HttpRequest struct */
 void

diff --git a/src/http.cc b/src/http.cc
index 999cc9a26cb65695190120857fd46823583dd4f8..5f38fd6c1f1eaa2706ed1c15abd97f1472dbe757 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -72,8 +72,8 @@ CBDATA_CLASS_INIT(HttpStateData);
 static const char *const crlf = "\r\n";
 
 static void httpMaybeRemovePublic(StoreEntry *, http_status);
-static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, String strConnection, HttpRequest * request, HttpRequest * orig_request,
-        HttpHeader * hdr_out, int we_do_ranges, http_state_flags);
+static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, const String strConnection, HttpRequest * request, const HttpRequest * orig_request,
+        HttpHeader * hdr_out, const int we_do_ranges, const http_state_flags);
 
 HttpStateData::HttpStateData(FwdState *theFwdState) : AsyncJob("HttpStateData"), ServerStateData(theFwdState),
         lastChunk(0), header_bytes_read(0), reply_bytes_read(0), httpChunkDecoder(NULL)
@@ -1647,20 +1647,22 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
     strConnection.clean();
 }
 
+/**
+ * Decides whether a particular header may be cloned from the received Clients request
+ * to our outgoing fetch request.
+ */
 void
-copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, String strConnection, HttpRequest * request, HttpRequest * orig_request, HttpHeader * hdr_out, int we_do_ranges, http_state_flags flags)
+copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, const String strConnection, HttpRequest * request, const HttpRequest * orig_request, HttpHeader * hdr_out, const int we_do_ranges, const http_state_flags flags)
 {
     debugs(11, 5, "httpBuildRequestHeader: " << e->name.buf() << ": " << e->value.buf());
 
-    if (!httpRequestHdrAllowed(e, &strConnection)) {
-        debugs(11, 2, "'" << e->name.buf() << "' header denied by anonymize_headers configuration");
-        return;
-    }
-
     switch (e->id) {
 
+/** \title RFC 2616 sect 13.5.1 - Hop-by-Hop headers which Squid should not pass on. */
+
     case HDR_PROXY_AUTHORIZATION:
-        /* Only pass on proxy authentication to peers for which
+        /** \par Proxy-Authorization:
+         * Only pass on proxy authentication to peers for which
          * authentication forwarding is explicitly enabled
          */
 
@@ -1672,16 +1674,31 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
 
         break;
 
+/** \title RFC 2616 sect 13.5.1 - Hop-by-Hop headers which Squid does not pass on. */
+
+    case HDR_CONNECTION:          /** \par Connection: */
+    case HDR_TE:                  /** \par TE: */
+    case HDR_KEEP_ALIVE:          /** \par Keep-Alive: */
+    case HDR_PROXY_AUTHENTICATE:  /** \par Proxy-Authenticate: */
+    case HDR_TRAILERS:            /** \par Trailers: */
+    case HDR_UPGRADE:             /** \par Upgrade: */
+    case HDR_TRANSFER_ENCODING:   /** \par Transfer-Encoding: */
+        break;
+
+
+/** \title OTHER headers I haven't bothered to track down yet. */
+
     case HDR_AUTHORIZATION:
-        /* Pass on WWW authentication */
+        /** \par WWW-Authorization:
+         * Pass on WWW authentication */
 
         if (!flags.originpeer) {
             hdr_out->addEntry(e->clone());
         } else {
-            /* In accelerators, only forward authentication if enabled
+            /** \note In accelerators, only forward authentication if enabled
+             * by login=PASS or login=PROXYPASS
              * (see also below for proxy->server authentication)
              */
-
             if (orig_request->peer_login &&
                     (strcmp(orig_request->peer_login, "PASS") == 0 ||
                      strcmp(orig_request->peer_login, "PROXYPASS") == 0)) {
@@ -1692,7 +1709,7 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
         break;
 
     case HDR_HOST:
-        /*
+        /** \par Host:
          * Normally Squid rewrites the Host: header.
          * However, there is one case when we don't: If the URL
          * went through our redirector and the admin configured
@@ -1717,8 +1734,9 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
         break;
 
     case HDR_IF_MODIFIED_SINCE:
-        /* append unless we added our own;
-         * note: at most one client's ims header can pass through */
+        /** \par If-Modified-Since:
+        * append unless we added our own;
+         * \note at most one client's ims header can pass through */
 
         if (!hdr_out->has(HDR_IF_MODIFIED_SINCE))
             hdr_out->addEntry(e->clone());
@@ -1726,6 +1744,8 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
         break;
 
     case HDR_MAX_FORWARDS:
+        /** \par Max-Forwards:
+         * pass only on TRACE requests */
         if (orig_request->method == METHOD_TRACE) {
             const int hops = e->getInt();
 
@@ -1736,7 +1756,9 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
         break;
 
     case HDR_VIA:
-        /* If Via is disabled then forward any received header as-is */
+        /** \par Via:
+         * If Via is disabled then forward any received header as-is.
+         * Otherwise leave for explicit updated addition later. */
 
         if (!Config.onoff.via)
             hdr_out->addEntry(e->clone());
@@ -1748,6 +1770,8 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
     case HDR_IF_RANGE:
 
     case HDR_REQUEST_RANGE:
+        /** \par Range:, If-Range:, Request-Range:
+         * Only pass if we accept ranges */
         if (!we_do_ranges)
             hdr_out->addEntry(e->clone());
 
@@ -1755,22 +1779,32 @@ copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, St
 
     case HDR_PROXY_CONNECTION:
 
-    case HDR_CONNECTION:
-
     case HDR_X_FORWARDED_FOR:
 
     case HDR_CACHE_CONTROL:
-        /* append these after the loop if needed */
+        /** \par Proxy-Connaction:, X-Forwarded-For:, Cache-Control:
+         * handled specially by Squid, so leave off for now.
+         * append these after the loop if needed */
         break;
 
     case HDR_FRONT_END_HTTPS:
+        /** \par Front-End-Https:
+         * Pass thru only if peer is configured with front-end-https */
         if (!flags.front_end_https)
             hdr_out->addEntry(e->clone());
 
         break;
 
     default:
-        /* pass on all other header fields */
+        /** \par default.
+         * pass on all other header fields
+         * which are NOT listed by the special Connection: header. */
+
+        if (strConnection.size()>0 && strListIsMember(&strConnection, e->name.buf(), ',')) {
+            debugs(11, 2, "'" << e->name.buf() << "' header cropped by Connection: definition");
+            return;
+        }
+
         hdr_out->addEntry(e->clone());
     }
 }