Add Proxy-Status header for header validation errors

Summary: Add Proxy-Status header (http_headers_parsing_error) for HTTP header validation errors to improve visibility in 400 errors.

Reviewed By: afrind

Differential Revision: D68592287

fbshipit-source-id: 2b157a7f04971e24b014d6f9e15f9cd68970f15d

third-party/proxygen/src/proxygen/lib/http/session/HTTPDirectResponseHandler.cpp

@@ -18,12 +18,10 @@ using std::unique_ptr;
 namespace proxygen {
 
 HTTPDirectResponseHandler::HTTPDirectResponseHandler(
-    unsigned statusCode,
-    const std::string& statusMsg,
-    const HTTPErrorPage* errorPage)
+    unsigned statusCode, std::string statusMsg, const HTTPErrorPage* errorPage)
     : txn_(nullptr),
       errorPage_(errorPage),
-      statusMessage_(statusMsg),
+      statusMessage_(std::move(statusMsg)),
       statusCode_(statusCode),
       headersSent_(false),
       eomSent_(false),
@@ -59,10 +57,11 @@ void HTTPDirectResponseHandler::onHeadersComplete(
   }
   if (errorPage_) {
     HTTPErrorPage::Page page = errorPage_->generate(
-        0, statusCode_, statusMessage_, nullptr, empty_string);
+        0, statusCode_, statusMessage_, nullptr, empty_string, err_);
     VLOG(4) << "sending error page with type " << page.contentType;
     response.getHeaders().add(HTTP_HEADER_CONTENT_TYPE, page.contentType);
     responseBody = std::move(page.content);
+    page.headers.copyTo(response.getHeaders());
   }
   response.getHeaders().add(
       HTTP_HEADER_CONTENT_LENGTH,
@@ -93,6 +92,10 @@ void HTTPDirectResponseHandler::onUpgrade(
 }
 
 void HTTPDirectResponseHandler::onError(const HTTPException& error) noexcept {
+  if (error.hasProxygenError()) {
+    err_ = error.getProxygenError();
+  }
+
   if (error.getDirection() == HTTPException::Direction::INGRESS) {
     if (error.getProxygenError() == kErrorTimeout) {
       VLOG(4) << "processing ingress timeout";

third-party/proxygen/src/proxygen/lib/http/session/HTTPDirectResponseHandler.h

@@ -17,7 +17,7 @@ class HTTPErrorPage;
 class HTTPDirectResponseHandler : public HTTPTransaction::Handler {
  public:
   HTTPDirectResponseHandler(unsigned statusCode,
-                            const std::string& statusMsg,
+                            std::string statusMsg,
                             const HTTPErrorPage* errorPage = nullptr);
 
   void forceConnectionClose(bool close) {
@@ -45,6 +45,7 @@ class HTTPDirectResponseHandler : public HTTPTransaction::Handler {
   const HTTPErrorPage* errorPage_;
   std::string statusMessage_;
   unsigned statusCode_;
+  ProxygenError err_{kErrorNone};
   bool headersSent_ : 1;
   bool eomSent_ : 1;
   bool forceConnectionClose_ : 1;

third-party/proxygen/src/proxygen/lib/http/session/HTTPErrorPage.cpp

@@ -24,9 +24,13 @@ HTTPErrorPage::Page HTTPStaticErrorPage::generate(
     unsigned /*httpStatusCode*/,
     const std::string& /*reason*/,
     std::unique_ptr<folly::IOBuf> /*body*/,
-    const std::string& /*detailReason*/) const {
+    const std::string& /*detailReason*/,
+    ProxygenError err) const {
 
-  return HTTPErrorPage::Page(contentType_, content_->clone());
+  HTTPHeaders headers;
+  VLOG(4) << "adding server-status header for proxygen error";
+  headers.set("Server-Status", folly::to<std::string>(static_cast<int>(err)));
+  return {contentType_, content_->clone(), std::move(headers)};
 }
 
 } // namespace proxygen

third-party/proxygen/src/proxygen/lib/http/session/HTTPErrorPage.h

@@ -10,6 +10,8 @@
 
 #include <cstdint>
 #include <memory>
+#include <proxygen/lib/http/HTTPHeaders.h>
+#include <proxygen/lib/http/ProxygenErrorEnum.h>
 #include <string>
 
 namespace folly {
@@ -30,12 +32,19 @@ class HTTPErrorPage {
         : contentType(pageContentType), content(std::move(pageContent)) {
     }
 
-    Page(Page&& other) noexcept
-        : contentType(other.contentType), content(std::move(other.content)) {
+    Page(std::string pageContentType,
+         std::unique_ptr<folly::IOBuf> pageContent,
+         HTTPHeaders pageHeaders)
+        : contentType(std::move(pageContentType)),
+          content(std::move(pageContent)),
+          headers(std::move(pageHeaders)) {
     }
 
+    Page(Page&& other) noexcept = default;
+
     const std::string contentType;
     std::unique_ptr<folly::IOBuf> content;
+    HTTPHeaders headers;
   };
 
   virtual ~HTTPErrorPage() {
@@ -45,7 +54,8 @@ class HTTPErrorPage {
                         unsigned httpStatusCode,
                         const std::string& reason,
                         std::unique_ptr<folly::IOBuf> body,
-                        const std::string& detailReason) const = 0;
+                        const std::string& detailReason,
+                        ProxygenError err) const = 0;
 };
 
 /**
@@ -61,9 +71,10 @@ class HTTPStaticErrorPage : public HTTPErrorPage {
                 unsigned httpStatusCode,
                 const std::string& reason,
                 std::unique_ptr<folly::IOBuf> body,
-                const std::string& detailReason) const override;
+                const std::string& detailReason,
+                ProxygenError err) const override;
 
- private:
+ protected:
   std::unique_ptr<folly::IOBuf> content_;
   std::string contentType_;
 };

third-party/proxygen/src/proxygen/lib/http/session/HTTPSessionAcceptor.cpp

@@ -43,11 +43,7 @@ HTTPSessionAcceptor::~HTTPSessionAcceptor() {
 
 const HTTPErrorPage* HTTPSessionAcceptor::getErrorPage(
     const SocketAddress& addr) const {
-  const HTTPErrorPage* errorPage = nullptr;
-  if (errorPage == nullptr) {
-    errorPage = defaultErrorPage_.get();
-  }
-  return errorPage;
+  return defaultErrorPage_.get();
 }
 
 void HTTPSessionAcceptor::onNewConnection(folly::AsyncTransport::UniquePtr sock,

third-party/proxygen/src/proxygen/lib/http/session/HTTPSessionAcceptor.h

@@ -58,7 +58,7 @@ class HTTPSessionAcceptor
    *          else the default error page generator if one has
    *          been set, or else nullptr.
    */
-  virtual const HTTPErrorPage* getErrorPage(
+  [[nodiscard]] virtual const HTTPErrorPage* getErrorPage(
       const folly::SocketAddress& addr) const;
 
   /**

third-party/proxygen/src/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp

@@ -25,6 +25,7 @@
 #include <proxygen/lib/http/observer/HTTPSessionObserverInterface.h>
 #include <proxygen/lib/http/session/HTTPDirectResponseHandler.h>
 #include <proxygen/lib/http/session/HTTPDownstreamSession.h>
+#include <proxygen/lib/http/session/HTTPErrorPage.h>
 #include <proxygen/lib/http/session/HTTPSession.h>
 #include <proxygen/lib/http/session/test/HTTPSessionMocks.h>
 #include <proxygen/lib/http/session/test/HTTPSessionTest.h>
@@ -1532,6 +1533,67 @@ TEST_F(HTTPDownstreamSessionTest, HttpWithAckTimingConnError) {
   httpSession_->timeoutExpired();
 }
 
+TEST_F(HTTPDownstreamSessionTest, ServerStatusHeaderOnError) {
+  // On ingress error, controller will call getParseErrorHandler, instantiating
+  // a DirectResponseHandler. The handler then calls generate() on a
+  // HTTPStaticErrorPage, which will set a Server-Status header to the
+  // corresponding ProxygenError.
+  HTTPStaticErrorPage page{folly::IOBuf::fromString("")};
+  HTTPTransaction::Handler* errorHandler =
+      new HTTPDirectResponseHandler(400, "Bad Request", &page);
+  EXPECT_CALL(mockController_, getParseErrorHandler(_, _, _))
+      .WillOnce(Return(errorHandler));
+
+  NiceMock<MockHTTPCodecCallback> callbacks;
+  clientCodec_->setCallback(&callbacks);
+
+  EXPECT_CALL(callbacks, onHeadersComplete(1, _))
+      .WillOnce(Invoke([](HTTPCodec::StreamID,
+                          std::shared_ptr<HTTPMessage> msg) {
+        EXPECT_EQ(msg->getHeaders().getSingleOrEmpty("Server-Status"), "16");
+      }));
+
+  auto req = getGetRequest("/");                 // construct basic get req
+  req.getHeaders().set("Host", " \xfa\r\n\r\n"); // set invalid header value in
+                                                 // host field
+  sendRequest(req);                              // send req with eom
+
+  folly::DelayedDestruction::DestructorGuard dg(httpSession_);
+  flushRequests();
+  eventBase_.loopOnce();
+  eventBase_.loopOnce();
+  parseOutput(*clientCodec_); // client parses resp with server-status set to
+                              // kErrorParseHeader (16)
+  expectDetachSession();
+}
+
+TEST_F(HTTP2DownstreamSessionTest, ServerStatusHeaderOnError) {
+  HTTPStaticErrorPage page{folly::IOBuf::fromString("")};
+  HTTPTransaction::Handler* errorHandler =
+      new HTTPDirectResponseHandler(400, "Bad Request", &page);
+  EXPECT_CALL(mockController_, getParseErrorHandler(_, _, _))
+      .WillOnce(Return(errorHandler));
+
+  NiceMock<MockHTTPCodecCallback> callbacks;
+  clientCodec_->setCallback(&callbacks);
+
+  EXPECT_CALL(callbacks, onHeadersComplete(1, _))
+      .WillOnce(Invoke([](HTTPCodec::StreamID,
+                          std::shared_ptr<HTTPMessage> msg) {
+        EXPECT_EQ(msg->getHeaders().getSingleOrEmpty("Server-Status"), "16");
+      }));
+
+  auto req = getGetRequest("/");
+  req.getHeaders().set("Host", " \xfa\r\n\r\n");
+  sendRequest(req);
+
+  flushRequests();
+  eventBase_.loopOnce();
+  eventBase_.loopOnce();
+  parseOutput(*clientCodec_);
+  gracefulShutdown();
+}
+
 TEST_F(HTTP2DownstreamSessionTest, TestPing) {
   // send a request with a PING, should get the PING first
   auto handler = addSimpleStrictHandler();