Update Host, Via, and other headers in-place when possible (#1203)

This change optimizes Host header synchronization code that usually does
not need to change the Host header field but was always deleting the old
field and generating/appending a new one. Appending Host also violated
an RFC 9110 section 7.2 rule (on behalf of the user agent whose Host we
were rewriting): "A user agent that sends Host SHOULD send it as the
first field in the header section".

Also, removing old header fields and appending new ones introduced HTTP
header changes that broke otherwise working peers. For example, some
origins denied Squid-proxied messages exclusively due to an unusual
(from those origins point of view) Host header position. This change
reduces (but does not eliminate) such unnecessary and risky changes.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index d70fed3c5a831543e3a1dff3b5af0c12815ea969..14b38e5fefa5f672cba185849e565600af340e96 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -153,6 +153,7 @@ Thank you!
     Eray Aslan <eray.aslan@caf.com.tr>
     Eray Aslan <eraya@a21an.org>
     Eric Stern <estern@logisense.com>
+    Eric Walters <walters.w.eric@gmail.com>
     Erik Hofman <erik.hofman@a1.nl>
     Eugene Gladchenko <eugene@donpac.ru>
     Evan Jones <ejones@uwaterloo.ca>

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index c1fbcd0160dfbe8117b1eb12cb436ea3835d3a1c..82c600381dd259a605471881384a77d83812b9dd 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 55    HTTP Header */
 
 #include "squid.h"
+#include "base/Assure.h"
 #include "base/CharacterSet.h"
 #include "base/EnumIterator.h"
 #include "base/Raw.h"
@@ -991,10 +992,7 @@ HttpHeader::addVia(const AnyP::ProtocolVersion &ver, const HttpHeader *from)
         if (!strVia.isEmpty())
             strVia.append(", ", 2);
         strVia.append(buf);
-        // XXX: putStr() still suffers from String size limits
-        Must(strVia.length() < String::SizeMaxXXX());
-        delById(Http::HdrType::VIA);
-        putStr(Http::HdrType::VIA, strVia.c_str());
+        updateOrAddStr(Http::HdrType::VIA, strVia);
     }
 }
 
@@ -1114,6 +1112,43 @@ HttpHeader::putExt(const char *name, const char *value)
     addEntry(new HttpHeaderEntry(Http::HdrType::OTHER, SBuf(name), value));
 }
 
+void
+HttpHeader::updateOrAddStr(const Http::HdrType id, const SBuf &newValue)
+{
+    assert(any_registered_header(id));
+    assert(Http::HeaderLookupTable.lookup(id).type == Http::HdrFieldType::ftStr);
+
+    // XXX: HttpHeaderEntry::value suffers from String size limits
+    Assure(newValue.length() < String::SizeMaxXXX());
+
+    if (!CBIT_TEST(mask, id)) {
+        auto newValueCopy = newValue; // until HttpHeaderEntry::value becomes SBuf
+        addEntry(new HttpHeaderEntry(id, SBuf(), newValueCopy.c_str()));
+        return;
+    }
+
+    auto foundSameName = false;
+    for (auto &e: entries) {
+        if (!e || e->id != id)
+            continue;
+
+        if (foundSameName) {
+            // get rid of this repeated same-name entry
+            delete e;
+            e = nullptr;
+            continue;
+        }
+
+        if (newValue.cmp(e->value.termedBuf()) != 0)
+            e->value.assign(newValue.rawContent(), newValue.plength());
+
+        foundSameName = true;
+        // continue to delete any repeated same-name entries
+    }
+    assert(foundSameName);
+    debugs(55, 5, "synced: " << Http::HeaderLookupTable.lookup(id).name << ": " << newValue);
+}
+
 int
 HttpHeader::getInt(Http::HdrType id) const
 {

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 7a5568785daf8f22850a4055b026dbb4350399ec..d60d1d555cf5224a3db3821f61c4f6f2828c2383 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -142,6 +142,11 @@ public:
     void putRange(const HttpHdrRange * range);
     void putSc(HttpHdrSc *sc);
     void putExt(const char *name, const char *value);
+
+    /// Ensures that the header has the given field, removing or replacing any
+    /// same-name fields with conflicting values as needed.
+    void updateOrAddStr(Http::HdrType, const SBuf &);
+
     int getInt(Http::HdrType id) const;
     int64_t getInt64(Http::HdrType id) const;
     time_t getTime(Http::HdrType id) const;

diff --git a/src/errorpage.cc b/src/errorpage.cc
index eab150d6fbf6f79bbdcfb3239b4eaa1c0bf93787..6d888abfe2ad53092622a2ae96bde46cded0dda9 100644 (file)
--- a/src/errorpage.cc
+++ b/src/errorpage.cc
@@ -1324,8 +1324,8 @@ ErrorState::BuildHttpReply()
          */
         if (!Config.errorDirectory) {
             /* We 'negotiated' this ONLY from the Accept-Language. */
-            rep->header.delById(Http::HdrType::VARY);
-            rep->header.putStr(Http::HdrType::VARY, "Accept-Language");
+            static const SBuf acceptLanguage("Accept-Language");
+            rep->header.updateOrAddStr(Http::HdrType::VARY, acceptLanguage);
         }
 
         /* add the Content-Language header according to RFC section 14.12 */

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 89ad44311faac6761471c7682e269d10dd987605..5c18648f81bfb8212ddd61c56d5321b069e5b547 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -1648,10 +1648,10 @@ Ftp::Server::setDataCommand()
     HttpRequest *const request = http->request;
     assert(request != nullptr);
     HttpHeader &header = request->header;
-    header.delById(Http::HdrType::FTP_COMMAND);
-    header.putStr(Http::HdrType::FTP_COMMAND, "PASV");
-    header.delById(Http::HdrType::FTP_ARGUMENTS);
-    header.putStr(Http::HdrType::FTP_ARGUMENTS, "");
+    static const SBuf pasvValue("PASV");
+    header.updateOrAddStr(Http::HdrType::FTP_COMMAND, pasvValue);
+    static const SBuf emptyValue("");
+    header.updateOrAddStr(Http::HdrType::FTP_ARGUMENTS, emptyValue);
     debugs(9, 5, "client data command converted to fake PASV");
 }
 

diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc
index 5e4007a75885573955157108328d295a6a86dee0..b7d8d3161b3af513e60a2de692e04da30fff9966 100644 (file)
--- a/src/servers/Http1Server.cc
+++ b/src/servers/Http1Server.cc
@@ -185,11 +185,8 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context)
     // some code still uses Host directly so normalize it using the previously
     // sanitized URL authority value.
     // For now preserve the case where Host is completely absent. That matters.
-    if (const auto x = request->header.delById(Http::HOST)) {
-        debugs(33, 5, "normalize " << x << " Host header using " << request->url.authority());
-        SBuf tmp(request->url.authority());
-        request->header.putStr(Http::HOST, tmp.c_str());
-    }
+    if (request->header.has(Http::HdrType::HOST))
+        request->header.updateOrAddStr(Http::HdrType::HOST, request->url.authority());
 
     // TODO: We fill request notes here until we find a way to verify whether
     // no ACL checking is performed before ClientHttpRequest::doCallouts().

diff --git a/src/tests/stub_HttpHeader.cc b/src/tests/stub_HttpHeader.cc
index d7c28c6a0adf3ac87f6fc84cbbd4ef8017c8d55e..d88fe67a3183fd11f2cef7f5a60ee8d617067f2e 100644 (file)
--- a/src/tests/stub_HttpHeader.cc
+++ b/src/tests/stub_HttpHeader.cc
@@ -64,6 +64,7 @@ void HttpHeader::putContRange(const HttpHdrContRange *) STUB
 void HttpHeader::putRange(const HttpHdrRange *) STUB
 void HttpHeader::putSc(HttpHdrSc *) STUB
 void HttpHeader::putExt(const char *, const char *) STUB
+void HttpHeader::updateOrAddStr(Http::HdrType, const SBuf &) STUB
 int HttpHeader::getInt(Http::HdrType) const STUB_RETVAL(0)
 int64_t HttpHeader::getInt64(Http::HdrType) const STUB_RETVAL(0)
 time_t HttpHeader::getTime(Http::HdrType) const STUB_RETVAL(0)