From: Eric Walters Date: Fri, 9 Dec 2022 05:58:43 +0000 (+0000) Subject: Update Host, Via, and other headers in-place when possible (#1203) X-Git-Tag: SQUID_6_0_1~59 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=05ba40fc1059ffe75c9a15a4e0c2923eca6801b2;p=thirdparty%2Fsquid.git 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 d70fed3c5a..14b38e5fef 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -153,6 +153,7 @@ Thank you! Eray Aslan Eray Aslan Eric Stern + Eric Walters Erik Hofman Eugene Gladchenko Evan Jones diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index c1fbcd0160..82c600381d 100644 --- 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 7a5568785d..d60d1d555c 100644 --- 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 eab150d6fb..6d888abfe2 100644 --- 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 89ad44311f..5c18648f81 100644 --- 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 5e4007a758..b7d8d3161b 100644 --- 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 d7c28c6a0a..d88fe67a31 100644 --- 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)