Skip to content

HTTP: update 1xx status handling #2048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions src/HttpRequest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -630,18 +630,6 @@ HttpRequest::ignoreRange(const char *reason)
// TODO: Some callers also delete HDR_RANGE, HDR_REQUEST_RANGE. Should we?
}

bool
HttpRequest::canHandle1xx() const
{
// old clients do not support 1xx unless they sent Expect: 100-continue
// (we reject all other Http::HdrType::EXPECT values so just check for Http::HdrType::EXPECT)
if (http_ver <= Http::ProtocolVersion(1,0) && !header.has(Http::HdrType::EXPECT))
return false;

// others must support 1xx control messages
return true;
}

Http::StatusCode
HttpRequest::checkEntityFraming() const
{
Expand Down
3 changes: 0 additions & 3 deletions src/HttpRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ class HttpRequest: public Http::Message

bool conditional() const; ///< has at least one recognized If-* header

/// whether the client is likely to be able to handle a 1xx reply
bool canHandle1xx() const;

/// \returns a pointer to a local static buffer containing request URI
/// that honors strip_query_terms and %-encodes unsafe URI characters
char *canonicalCleanUrl() const;
Expand Down
77 changes: 55 additions & 22 deletions src/http.cc
Original file line number Diff line number Diff line change
Expand Up @@ -761,14 +761,32 @@ HttpStateData::handle1xx(const HttpReply::Pointer &reply)
Must(!flags.serverSwitchedProtocols);
flags.serverSwitchedProtocols = (statusCode == Http::scSwitchingProtocols);

if (statusCode == Http::scContinue && request->forcedBodyContinuation)
return drop1xx("we have sent it already");
// old clients do not support 1xx unless they sent Expect: 100-continue
if (statusCode == Http::scContinue) {
// traffic optimization
if (request->forcedBodyContinuation)
return drop1xx("we have sent 100-continue already");
// tolerate HTTP/1.0 clients sending "Expect: 100-continue",
// and broken servers emitting 100 status without receiving Expect.
if (!request->header.has(Http::HdrType::EXPECT)) {
return drop1xx("the client did not request 100-continue");
}
else if (port->actAsOrigin) {
// RFC 9110 section 15.2:
// a server MUST NOT send a 1xx response to an HTTP/1.0 client
static const h1 = Http::ProtocolVersion(1,0);
if (port->transport <= h1 || request->http_ver <= h1)
return drop1xx("HTTP/1.0 protocol does not support 1xx status");
}
// else; RFC 9110 section 15.2: A proxy MUST forward 1xx responses

if (!request->canHandle1xx())
return drop1xx("the client does not support it");
if (flags.serverSwitchedProtocols) {
if (const auto reason = blockSwitchingProtocols(*reply))
return drop1xx(reason);
}

#if USE_HTTP_VIOLATIONS
// check whether the 1xx response forwarding is allowed by squid.conf
// check whether the 1xx response forwarding is denied by squid.conf
if (Config.accessList.reply) {
ACLFilledChecklist ch(Config.accessList.reply, originalRequest().getRaw());
ch.updateAle(fwd->al);
Expand All @@ -779,11 +797,6 @@ HttpStateData::handle1xx(const HttpReply::Pointer &reply)
}
#endif // USE_HTTP_VIOLATIONS

if (flags.serverSwitchedProtocols) {
if (const auto reason = blockSwitchingProtocols(*reply))
return drop1xx(reason);
}

debugs(11, 2, "forwarding 1xx to client");

// the Sink will use this to call us back after writing 1xx to the client
Expand All @@ -808,7 +821,7 @@ HttpStateData::drop1xx(const char *reason)
const auto err = new ErrorState(ERR_INVALID_RESP, Http::scBadGateway, request.getRaw(), fwd->al);
fwd->fail(err);
closeServer();
mustStop("prohibited HTTP/101 response");
mustStop("prohibited HTTP 101 response");
return;
}

Expand All @@ -824,21 +837,41 @@ HttpStateData::blockSwitchingProtocols(const HttpReply &reply) const
if (!upgradeHeaderOut)
return "Squid offered no Upgrade at all, but server switched to a tunnel";

// See RFC 7230 section 6.7 for the corresponding MUSTs

if (!reply.header.has(Http::HdrType::UPGRADE))
return "server did not send an Upgrade header field";
// RFC 9110 section 7.8 paragraph 12:
// A server that receives an Upgrade header field in an
// HTTP/1.0 request MUST ignore that Upgrade field.
if (request->http_ver <= Http::ProtocolVersion(1,0))
return "HTTP/1.0 does not suport Upgrade header field";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would Squid ever send a request that violates this MUST? If this check is really needed, it should go into request sending code, not response receiving code. If our sending code is correct, and we want to police versions in this case, then we can assert that the version is OK here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK today an ICAP/eCAP service can add Upgrade values which are blindly copied for delivery.
In future we may also want to proactively offer upgrades to better protocols (eg HTTPS, HTTP/2, etc).

This being a MUST requirement, IMO its worth checking to ensure we do not accidentally violate it.


// RFC 9110 section 7.8 paragraph 12:
// A sender of Upgrade MUST also send an "Upgrade" connection option
// in the Connection header field
if (!reply.header.hasListMember(Http::HdrType::CONNECTION, "upgrade", ','))
return "server did not send 'Connection: upgrade'";

const auto acceptedProtos = reply.header.getList(Http::HdrType::UPGRADE);
const char *pos = nullptr;
const char *accepted = nullptr;
int acceptedLen = 0;
while (strListGetItem(&acceptedProtos, ',', &accepted, &acceptedLen, &pos)) {
debugs(11, 5, "server accepted at least" << Raw(nullptr, accepted, acceptedLen));
return nullptr; // OK: let the client validate server's selection
if (reply.header.has(Http::HdrType::UPGRADE)) {
// RFC 9110 section 7.8 paragraph 5:
// A server that sends a 101 (Switching Protocols) response MUST send
// an Upgrade header field to indicate the new protocol(s) to which
// the connection is being switched
const auto acceptedProtos = reply.header.getList(Http::HdrType::UPGRADE);
const char *pos = nullptr;
const char *accepted = nullptr;
int acceptedLen = 0;
while (strListGetItem(&acceptedProtos, ',', &accepted, &acceptedLen, &pos)) {
debugs(11, 5, "server accepted at least" << Raw(nullptr, accepted, acceptedLen));
return nullptr; // OK: let the client validate server's selection
}

// RFC 9110 section 7.8 paragraph 5:
// A server MUST NOT switch to a protocol that was not indicated
// by the client in the corresponding request's Upgrade header field.
// TODO: abort when server violates this condition.

} else {
// RFC 9110 section 15.2.2:
// The server MUST generate an Upgrade header field in the response
return "server did not send an Upgrade header field";
}

return "server sent an essentially empty Upgrade header field";
Expand Down
1 change: 0 additions & 1 deletion src/tests/stub_HttpRequest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ void HttpRequest::initHTTP(const HttpRequestMethod &, AnyP::ProtocolType, const
HttpRequest * HttpRequest::clone() const STUB_RETVAL(nullptr)
bool HttpRequest::maybeCacheable() STUB_RETVAL(false)
bool HttpRequest::conditional() const STUB_RETVAL(false)
bool HttpRequest::canHandle1xx() const STUB_RETVAL(false)
char * HttpRequest::canonicalCleanUrl() const STUB_RETVAL(nullptr)
#if USE_ADAPTATION
Adaptation::History::Pointer HttpRequest::adaptLogHistory() const STUB_RETVAL(Adaptation::History::Pointer())
Expand Down
Loading