PR feedback for #2838

Author: John Luo

src/Kestrel.Core/Http2Limits.cs

@@ -13,11 +13,7 @@ public class Http2Limits
     {
         private int _maxStreamsPerConnection = 100;
         private int _headerTableSize = (int)Http2PeerSettings.DefaultHeaderTableSize;
-        private int _maxFrameSize = MinAllowedMaxFrameSize;
-
-        // These are limits defined by the RFC https://tools.ietf.org/html/rfc7540#section-4.2
-        public const int MinAllowedMaxFrameSize = 16 * 1024;
-        public const int MaxAllowedMaxFrameSize = 16 * 1024 * 1024 - 1;
+        private int _maxFrameSize = (int)Http2PeerSettings.DefaultMaxFrameSize;
 
         /// <summary>
         /// Limits the number of concurrent request streams per HTTP/2 connection. Excess streams will be refused.
@@ -42,7 +38,7 @@ public int MaxStreamsPerConnection
         /// <summary>
         /// Limits the size of the header compression table, in octets, the HPACK decoder on the server can use.
         /// <para>
-        /// Defaults to 4096
+        /// Value must be greater than 0, defaults to 4096
         /// </para>
         /// </summary>
         public int HeaderTableSize
@@ -62,17 +58,17 @@ public int HeaderTableSize
         /// <summary>
         /// Indicates the size of the largest frame payload that is allowed to be received, in octets. The size must be between 2^14 and 2^24-1.
         /// <para>
-        /// Defaults to 2^14 (16,384)
+        /// Value must be between 2^14 and 2^24, defaults to 2^14 (16,384)
         /// </para>
         /// </summary>
         public int MaxFrameSize
         {
             get => _maxFrameSize;
             set
             {
-                if (value < MinAllowedMaxFrameSize || value > MaxAllowedMaxFrameSize)
+                if (value < Http2PeerSettings.MinAllowedMaxFrameSize || value > Http2PeerSettings.MaxAllowedMaxFrameSize)
                 {
-                    throw new ArgumentOutOfRangeException(nameof(value), value, CoreStrings.FormatArgumentOutOfRange(MinAllowedMaxFrameSize, MaxAllowedMaxFrameSize));
+                    throw new ArgumentOutOfRangeException(nameof(value), value, CoreStrings.FormatArgumentOutOfRange(Http2PeerSettings.MinAllowedMaxFrameSize, Http2PeerSettings.MaxAllowedMaxFrameSize));
                 }
 
                 _maxFrameSize = value;

src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs

@@ -21,7 +21,7 @@ public class Http2FrameWriter
         // Literal Header Field without Indexing - Indexed Name (Index 8 - :status)
         private static readonly byte[] _continueBytes = new byte[] { 0x08, 0x03, (byte)'1', (byte)'0', (byte)'0' };
 
-        private uint _maxFrameSize = Http2Limits.MinAllowedMaxFrameSize;
+        private uint _maxFrameSize = Http2PeerSettings.MinAllowedMaxFrameSize;
         private Http2Frame _outgoingFrame;
         private readonly object _writeLock = new object();
         private readonly HPackEncoder _hpackEncoder = new HPackEncoder();

src/Kestrel.Core/Internal/Http2/Http2PeerSettings.cs

@@ -12,9 +12,11 @@ public class Http2PeerSettings
         public const bool DefaultEnablePush = true;
         public const uint DefaultMaxConcurrentStreams = uint.MaxValue;
         public const uint DefaultInitialWindowSize = 65535;
-        public const uint DefaultMaxFrameSize = Http2Limits.MinAllowedMaxFrameSize;
+        public const uint DefaultMaxFrameSize = MinAllowedMaxFrameSize;
         public const uint DefaultMaxHeaderListSize = uint.MaxValue;
         public const uint MaxWindowSize = int.MaxValue;
+        internal const int MinAllowedMaxFrameSize = 16 * 1024;
+        internal const int MaxAllowedMaxFrameSize = 16 * 1024 * 1024 - 1;
 
         public uint HeaderTableSize { get; set; } = DefaultHeaderTableSize;
 
@@ -64,11 +66,11 @@ public void Update(IList<Http2PeerSetting> settings)
                         InitialWindowSize = value;
                         break;
                     case Http2SettingsParameter.SETTINGS_MAX_FRAME_SIZE:
-                        if (value < Http2Limits.MinAllowedMaxFrameSize || value > Http2Limits.MaxAllowedMaxFrameSize)
+                        if (value < MinAllowedMaxFrameSize || value > MaxAllowedMaxFrameSize)
                         {
                             throw new Http2SettingsParameterOutOfRangeException(Http2SettingsParameter.SETTINGS_MAX_FRAME_SIZE,
-                                lowerBound: Http2Limits.MinAllowedMaxFrameSize,
-                                upperBound: Http2Limits.MaxAllowedMaxFrameSize);
+                                lowerBound: MinAllowedMaxFrameSize,
+                                upperBound: MaxAllowedMaxFrameSize);
                         }
 
                         MaxFrameSize = value;

test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs

@@ -76,28 +76,28 @@ public class Http2ConnectionTests : Http2TestBase
         private static readonly byte[] _worldBytes = Encoding.ASCII.GetBytes("world");
         private static readonly byte[] _helloWorldBytes = Encoding.ASCII.GetBytes("hello, world");
         private static readonly byte[] _noData = new byte[0];
-        private static readonly byte[] _maxData = Encoding.ASCII.GetBytes(new string('a', Http2Limits.MinAllowedMaxFrameSize));
+        private static readonly byte[] _maxData = Encoding.ASCII.GetBytes(new string('a', Http2PeerSettings.MinAllowedMaxFrameSize));
 
         [Fact]
         public async Task Frame_Received_OverMaxSize_FrameError()
         {
             await InitializeConnectionAsync(_echoApplication);
 
             await StartStreamAsync(1, _browserRequestHeaders, endStream: false);
-            uint length = Http2Limits.MinAllowedMaxFrameSize + 1;
+            uint length = Http2PeerSettings.MinAllowedMaxFrameSize + 1;
             await SendDataAsync(1, new byte[length].AsSpan(), endStream: true);
 
             await WaitForConnectionErrorAsync<Http2ConnectionErrorException>(
                 ignoreNonGoAwayFrames: true,
                 expectedLastStreamId: 1,
                 expectedErrorCode: Http2ErrorCode.FRAME_SIZE_ERROR,
-                expectedErrorMessage: CoreStrings.FormatHttp2ErrorFrameOverLimit(length, Http2Limits.MinAllowedMaxFrameSize));
+                expectedErrorMessage: CoreStrings.FormatHttp2ErrorFrameOverLimit(length, Http2PeerSettings.MinAllowedMaxFrameSize));
         }
 
         [Fact]
         public async Task ServerSettings_ChangesRequestMaxFrameSize()
         {
-            var length = Http2Limits.MinAllowedMaxFrameSize + 10;
+            var length = Http2PeerSettings.MinAllowedMaxFrameSize + 10;
             _connectionContext.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize = length;
             _connection = new Http2Connection(_connectionContext);
 
@@ -112,11 +112,11 @@ await ExpectAsync(Http2FrameType.HEADERS,
                 withStreamId: 1);
             // The client's settings is still defaulted to Http2PeerSettings.MinAllowedMaxFrameSize so the echo response will come back in two separate frames
             await ExpectAsync(Http2FrameType.DATA,
-                withLength: Http2Limits.MinAllowedMaxFrameSize,
+                withLength: Http2PeerSettings.MinAllowedMaxFrameSize,
                 withFlags: (byte)Http2DataFrameFlags.NONE,
                 withStreamId: 1);
             await ExpectAsync(Http2FrameType.DATA,
-                withLength: length - Http2Limits.MinAllowedMaxFrameSize,
+                withLength: length - Http2PeerSettings.MinAllowedMaxFrameSize,
                 withFlags: (byte)Http2DataFrameFlags.NONE,
                 withStreamId: 1);
             await ExpectAsync(Http2FrameType.DATA,
@@ -2139,7 +2139,7 @@ public async Task SETTINGS_ACK_Received_DoesNotSend_ACK()
         {
             await InitializeConnectionAsync(_noopApplication);
 
-            var frame = new Http2Frame(Http2Limits.MinAllowedMaxFrameSize);
+            var frame = new Http2Frame(Http2PeerSettings.MinAllowedMaxFrameSize);
             frame.PrepareSettings(Http2SettingsFrameFlags.ACK);
             await SendAsync(frame.Raw);
 
@@ -2262,7 +2262,7 @@ public async Task SETTINGS_Received_ChangesAllowedResponseMaxFrameSize()
         {
             // This includes the default response headers such as :status, etc
             var defaultResponseHeaderLength = 37;
-            var headerValueLength = Http2Limits.MinAllowedMaxFrameSize;
+            var headerValueLength = Http2PeerSettings.MinAllowedMaxFrameSize;
             // First byte is always 0
             // Second byte is the length of header name which is 1
             // Third byte is the header name which is A/B