From 2174ac22d251c60c85834f232f9a09ae94de65cc Mon Sep 17 00:00:00 2001
From: Alan Frindell <afrind@fb.com>
Date: Tue, 16 Jun 2015 15:37:14 -0700
Subject: [PATCH] Don't generate or accept SETTINGS with ENABLE_PUSH for
 servers

Summary: This is a violation of the spec.  Reported by Apple

Test Plan: New unit test

Reviewed By: xiaosophiawang@fb.com

Subscribers: xiaosophiawang, noamler, bmatheny

FB internal diff: D2160494

Tasks: 7431234
---
 .../http/codec/experimental/HTTP2Codec.cpp    | 13 +++++++++--
 .../lib/http/codec/experimental/HTTP2Codec.h  |  2 +-
 .../experimental/test/HTTP2CodecTest.cpp      | 22 ++++++++++++++++++-
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/proxygen/lib/http/codec/experimental/HTTP2Codec.cpp b/proxygen/lib/http/codec/experimental/HTTP2Codec.cpp
index 3b5d06ad2..9451b8bc1 100644
--- a/proxygen/lib/http/codec/experimental/HTTP2Codec.cpp
+++ b/proxygen/lib/http/codec/experimental/HTTP2Codec.cpp
@@ -534,7 +534,9 @@ ErrorCode HTTP2Codec::parseSettings(Cursor& cursor) {
         headerCodec_.setEncoderHeaderTableSize(setting.second);
         break;
       case SettingsId::ENABLE_PUSH:
-        if (setting.second != 0 && setting.second != 1) {
+        if ((setting.second != 0 && setting.second != 1) ||
+            (setting.second == 1 &&
+             transportDirection_ == TransportDirection::UPSTREAM)) {
           VLOG(4) << "Invalid ENABLE_PUSH setting=" << setting.second;
           return ErrorCode::PROTOCOL_ERROR;
         }
@@ -934,12 +936,19 @@ size_t HTTP2Codec::generateSettings(folly::IOBufQueue& writeBuf) {
   std::deque<SettingPair> settings;
   for (auto& setting: egressSettings_.getAllSettings()) {
     if (setting.isSet) {
-      settings.push_back(SettingPair(setting.id, setting.value));
       if (setting.id == SettingsId::HEADER_TABLE_SIZE) {
         headerCodec_.setDecoderHeaderTableMaxSize(setting.value);
       } else if (setting.id == SettingsId::MAX_HEADER_LIST_SIZE) {
         headerCodec_.setMaxUncompressed(setting.value);
+      } else if (setting.id == SettingsId::ENABLE_PUSH) {
+        if (transportDirection_ == TransportDirection::DOWNSTREAM) {
+          // HTTP/2 spec says downstream must not enable push
+          CHECK_EQ(setting.value, 0);
+        } else {
+          CHECK(setting.value == 0 || setting.value == 1);
+        }
       }
+      settings.push_back(SettingPair(setting.id, setting.value));
     }
   }
   VLOG(4) << "generating " << (unsigned)settings.size() << " settings";
diff --git a/proxygen/lib/http/codec/experimental/HTTP2Codec.h b/proxygen/lib/http/codec/experimental/HTTP2Codec.h
index 1f556385e..65b456949 100644
--- a/proxygen/lib/http/codec/experimental/HTTP2Codec.h
+++ b/proxygen/lib/http/codec/experimental/HTTP2Codec.h
@@ -194,7 +194,7 @@ public:
   };
   HTTPSettings egressSettings_{
     { SettingsId::HEADER_TABLE_SIZE, 4096 },
-    { SettingsId::ENABLE_PUSH, 1 },
+    { SettingsId::ENABLE_PUSH, 0 },
     { SettingsId::MAX_FRAME_SIZE, 16384 },
     { SettingsId::MAX_HEADER_LIST_SIZE, 1 << 17 }, // same as SPDYCodec
   };
diff --git a/proxygen/lib/http/codec/experimental/test/HTTP2CodecTest.cpp b/proxygen/lib/http/codec/experimental/test/HTTP2CodecTest.cpp
index 72d44d86b..6305b47cd 100644
--- a/proxygen/lib/http/codec/experimental/test/HTTP2CodecTest.cpp
+++ b/proxygen/lib/http/codec/experimental/test/HTTP2CodecTest.cpp
@@ -722,6 +722,24 @@ TEST_F(HTTP2CodecTest, BadSettings) {
   EXPECT_EQ(callbacks_.sessionErrors, 1);
 }
 
+TEST_F(HTTP2CodecTest, BadPushSettings) {
+  auto settings = downstreamCodec_.getEgressSettings();
+  settings->clearSettings();
+  settings->setSetting(SettingsId::ENABLE_PUSH, 0);
+  SetUpUpstreamTest();
+
+  parseUpstream([&] (IOBuf* ingress) {
+      // set ENABLE_PUSH to 1
+      folly::io::RWPrivateCursor c(ingress);
+      c.skip(http2::kFrameHeaderSize + sizeof(uint16_t));
+      c.writeBE<uint32_t>(1);
+    });
+  EXPECT_EQ(callbacks_.settings, 0);
+  EXPECT_EQ(callbacks_.streamErrors, 0);
+  EXPECT_EQ(callbacks_.sessionErrors, 1);
+}
+
+
 TEST_F(HTTP2CodecTest, SettingsTableSize) {
   auto settings = upstreamCodec_.getEgressSettings();
   settings->setSetting(SettingsId::HEADER_TABLE_SIZE, 8192);
@@ -789,6 +807,8 @@ TEST_F(HTTP2CodecTest, BasicPriority) {
 }
 
 TEST_F(HTTP2CodecTest, BasicPushPromise) {
+  auto settings = upstreamCodec_.getEgressSettings();
+  settings->setSetting(SettingsId::ENABLE_PUSH, 1);
   SetUpUpstreamTest();
   HTTPMessage req = getGetRequest();
   req.getHeaders().add("user-agent", "coolio");
@@ -802,12 +822,12 @@ TEST_F(HTTP2CodecTest, BasicPushPromise) {
 }
 
 TEST_F(HTTP2CodecTest, BadPushPromise) {
+  // ENABLE_PUSH is now 0 by default
   SetUpUpstreamTest();
   HTTPMessage req = getGetRequest();
   req.getHeaders().add("user-agent", "coolio");
   downstreamCodec_.generateHeader(output_, 2, req, 1);
 
-  upstreamCodec_.getEgressSettings()->setSetting(SettingsId::ENABLE_PUSH, 0);
   parseUpstream();
   EXPECT_EQ(callbacks_.messageBegin, 0);
   EXPECT_EQ(callbacks_.headersComplete, 0);