[ALPS] Add feature gates to parsing code

We should have an overall, and a per frame killswitch we can use in
finch when needed. This should help address future issues.

Bug: 1355247
Change-Id: Idce0c66f3c0754e3268b5111759afc55b1394a6d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3845793
Commit-Queue: David Schinazi <dschinazi@chromium.org>
Reviewed-by: David Schinazi <dschinazi@chromium.org>
Auto-Submit: Ari Chivukula <arichiv@chromium.org>
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1037842}

net/base/features.cc

@@ -344,4 +344,10 @@ const base::FeatureParam<bool> kStorageAccessAPIGrantsUnpartitionedStorage(
 // by the top level site to reduce fingerprinting.
 const base::Feature kThirdPartyStoragePartitioning{
     "ThirdPartyStoragePartitioning", base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::Feature kAlpsParsing{"AlpsParsing",
+                                 base::FEATURE_ENABLED_BY_DEFAULT};
+
+const base::Feature kAlpsClientHintParsing{"AlpsClientHintParsing",
+                                           base::FEATURE_ENABLED_BY_DEFAULT};
 }  // namespace net::features

net/base/features.h

@@ -487,6 +487,12 @@ NET_EXPORT extern const base::FeatureParam<bool>
 
 NET_EXPORT extern const base::Feature kThirdPartyStoragePartitioning;
 
+// Whether ALPS parsing is on for any type of frame.
+NET_EXPORT extern const base::Feature kAlpsParsing;
+
+// Whether ALPS parsing is on for client hint parsing specifically.
+NET_EXPORT extern const base::Feature kAlpsClientHintParsing;
+
 }  // namespace net::features
 
 #endif  // NET_BASE_FEATURES_H_

net/spdy/alps_decoder.cc

@@ -4,6 +4,9 @@
 
 #include "net/spdy/alps_decoder.h"
 
+#include "base/feature_list.h"
+#include "net/base/features.h"
+
 namespace net {
 namespace {
 
@@ -101,23 +104,34 @@ bool AlpsDecoder::AcceptChParser::OnFrameHeader(spdy::SpdyStreamId stream_id,
                                                 size_t length,
                                                 uint8_t type,
                                                 uint8_t flags) {
-  if (type != static_cast<uint8_t>(spdy::SpdyFrameType::ACCEPT_CH) ||
-      error_ != Error::kNoError) {
-    // Ignore every frame except for ACCEPT_CH.
-    // Ignore data after an error has occurred.
-    // Returning false causes Http2DecoderAdapter not to call OnFramePayload().
-    return false;
-  }
-  if (stream_id != 0) {
-    error_ = Error::kAcceptChInvalidStream;
+  // Ignore data after an error has occurred.
+  if (error_ != Error::kNoError)
     return false;
-  }
-  if (flags != 0) {
-    error_ = Error::kAcceptChWithFlags;
+  // Stop all alps parsing if it's disabled.
+  if (!base::FeatureList::IsEnabled(features::kAlpsParsing))
     return false;
+  // Handle per-type parsing.
+  switch (type) {
+    case static_cast<uint8_t>(spdy::SpdyFrameType::ACCEPT_CH): {
+      // Stop alps client hint parsing if it's disabled.
+      if (!base::FeatureList::IsEnabled(features::kAlpsClientHintParsing))
+        return false;
+      // Check for issues with the frame.
+      if (stream_id != 0) {
+        error_ = Error::kAcceptChInvalidStream;
+        return false;
+      }
+      if (flags != 0) {
+        error_ = Error::kAcceptChWithFlags;
+        return false;
+      }
+      // This frame can be parsed in OnFramePayload.
+      return true;
+    }
+    default:
+      // Ignore all other types.
+      return false;
   }
-
-  return true;
 }
 
 void AlpsDecoder::AcceptChParser::OnFramePayload(const char* data, size_t len) {

net/spdy/alps_decoder_test.cc

@@ -4,6 +4,8 @@
 
 #include "net/spdy/alps_decoder.h"
 
+#include "base/test/scoped_feature_list.h"
+#include "net/base/features.h"
 #include "net/base/hex_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -123,6 +125,58 @@ TEST(AlpsDecoderTest, ParseLargeAcceptChFrame) {
                                                   accept_ch_tokens}));
 }
 
+TEST(AlpsDecoderTest, DisableAlpsParsing) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(features::kAlpsParsing);
+  AlpsDecoder decoder;
+  AlpsDecoder::Error error = decoder.Decode(HexDecode(
+      // ACCEPT_CH frame
+      "00003d"                    // length
+      "89"                        // type ACCEPT_CH
+      "00"                        // flags
+      "00000000"                  // stream ID
+      "0017"                      // origin length
+      "68747470733a2f2f7777772e"  //
+      "6578616d706c652e636f6d"    // origin "https://www.example.com"
+      "0003"                      // value length
+      "666f6f"                    // value "foo"
+      "0018"                      // origin length
+      "68747470733a2f2f6d61696c"  //
+      "2e6578616d706c652e636f6d"  // origin "https://mail.example.com"
+      "0003"                      // value length
+      "626172"                    // value "bar"
+      ));
+
+  EXPECT_EQ(AlpsDecoder::Error::kNoError, error);
+  EXPECT_THAT(decoder.GetAcceptCh(), IsEmpty());
+}
+
+TEST(AlpsDecoderTest, DisableAlpsClientHintParsing) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(features::kAlpsClientHintParsing);
+  AlpsDecoder decoder;
+  AlpsDecoder::Error error = decoder.Decode(HexDecode(
+      // ACCEPT_CH frame
+      "00003d"                    // length
+      "89"                        // type ACCEPT_CH
+      "00"                        // flags
+      "00000000"                  // stream ID
+      "0017"                      // origin length
+      "68747470733a2f2f7777772e"  //
+      "6578616d706c652e636f6d"    // origin "https://www.example.com"
+      "0003"                      // value length
+      "666f6f"                    // value "foo"
+      "0018"                      // origin length
+      "68747470733a2f2f6d61696c"  //
+      "2e6578616d706c652e636f6d"  // origin "https://mail.example.com"
+      "0003"                      // value length
+      "626172"                    // value "bar"
+      ));
+
+  EXPECT_EQ(AlpsDecoder::Error::kNoError, error);
+  EXPECT_THAT(decoder.GetAcceptCh(), IsEmpty());
+}
+
 TEST(AlpsDecoderTest, IncompleteFrame) {
   AlpsDecoder decoder;
   AlpsDecoder::Error error =